From b040fb1c73927591deb5d403b0e418aefa037b41 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Wed, 17 Sep 2025 15:32:11 +0200 Subject: [PATCH 1/4] feat(AppFramework): Add missing NoTwoFactorRequired attribute It's in our documentation but was never implemented. Signed-off-by: Carl Schwan --- .../lib/Controller/ThemingController.php | 14 +-- core/Controller/CSRFTokenController.php | 4 +- core/Controller/JsController.php | 12 +- core/Controller/OCJSController.php | 5 +- .../TwoFactorChallengeController.php | 32 ++--- core/Middleware/TwoFactorMiddleware.php | 37 +++++- lib/composer/composer/autoload_classmap.php | 2 + lib/composer/composer/autoload_static.php | 2 + .../Attributes/TwoFactorSetUpDoneRequired.php | 18 +++ .../Http/Attribute/NoTwoFactorRequired.php | 25 ++++ .../Middleware/TwoFactorMiddlewareTest.php | 119 ++++++++++-------- 11 files changed, 169 insertions(+), 101 deletions(-) create mode 100644 lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php create mode 100644 lib/public/AppFramework/Http/Attribute/NoTwoFactorRequired.php diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php index ff8012c4ecd..a581605c374 100644 --- a/apps/theming/lib/Controller/ThemingController.php +++ b/apps/theming/lib/Controller/ThemingController.php @@ -17,6 +17,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting; use OCP\AppFramework\Http\Attribute\BruteForceProtection; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoTwoFactorRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\ContentSecurityPolicy; @@ -61,13 +62,10 @@ class ThemingController extends Controller { } /** - * @param string $setting - * @param string $value - * @return DataResponse * @throws NotPermittedException */ #[AuthorizedAdminSetting(settings: Admin::class)] - public function updateStylesheet($setting, $value) { + public function updateStylesheet(string $setting, string $value): DataResponse { $value = trim($value); $error = null; $saved = false; @@ -167,13 +165,10 @@ class ThemingController extends Controller { } /** - * @param string $setting - * @param mixed $value - * @return DataResponse * @throws NotPermittedException */ #[AuthorizedAdminSetting(settings: Admin::class)] - public function updateAppMenu($setting, $value) { + public function updateAppMenu(string $setting, mixed $value): DataResponse { $error = null; switch ($setting) { case 'defaultApps': @@ -218,7 +213,6 @@ class ThemingController extends Controller { } /** - * @return DataResponse * @throws NotPermittedException */ #[AuthorizedAdminSetting(settings: Admin::class)] @@ -384,7 +378,6 @@ class ThemingController extends Controller { /** * @NoSameSiteCookieRequired - * @NoTwoFactorRequired * * Get the CSS stylesheet for a theme * @@ -398,6 +391,7 @@ class ThemingController extends Controller { */ #[PublicPage] #[NoCSRFRequired] + #[NoTwoFactorRequired] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] public function getThemeStylesheet(string $themeId, bool $plain = false, bool $withCustomCss = false) { $themes = $this->themesService->getThemes(); diff --git a/core/Controller/CSRFTokenController.php b/core/Controller/CSRFTokenController.php index edf7c26e94c..85b187f42e6 100644 --- a/core/Controller/CSRFTokenController.php +++ b/core/Controller/CSRFTokenController.php @@ -13,6 +13,7 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoTwoFactorRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\JSONResponse; @@ -34,13 +35,12 @@ class CSRFTokenController extends Controller { * * 200: CSRF token returned * 403: Strict cookie check failed - * - * @NoTwoFactorRequired */ #[PublicPage] #[NoCSRFRequired] #[FrontpageRoute(verb: 'GET', url: '/csrftoken')] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] + #[NoTwoFactorRequired] public function index(): JSONResponse { if (!$this->request->passesStrictCookieCheck()) { return new JSONResponse([], Http::STATUS_FORBIDDEN); diff --git a/core/Controller/JsController.php b/core/Controller/JsController.php index bb40a4f2117..b74e0583683 100644 --- a/core/Controller/JsController.php +++ b/core/Controller/JsController.php @@ -13,6 +13,7 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoTwoFactorRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\FileDisplayResponse; @@ -42,16 +43,15 @@ class JsController extends Controller { /** * @NoSameSiteCookieRequired - * @NoTwoFactorRequired * * @param string $fileName js filename with extension * @param string $appName js folder name - * @return FileDisplayResponse|NotFoundResponse */ #[PublicPage] #[NoCSRFRequired] #[FrontpageRoute(verb: 'GET', url: '/js/{appName}/{fileName}')] - public function getJs(string $fileName, string $appName): Response { + #[NoTwoFactorRequired] + public function getJs(string $fileName, string $appName): FileDisplayResponse|NotFoundResponse { try { $folder = $this->appData->getFolder($appName); $gzip = false; @@ -76,15 +76,11 @@ class JsController extends Controller { } /** - * @NoTwoFactorRequired - * - * @param ISimpleFolder $folder - * @param string $fileName * @param bool $gzip is set to true if we use the gzip file - * @return ISimpleFile * * @throws NotFoundException */ + #[NoTwoFactorRequired] private function getFile(ISimpleFolder $folder, string $fileName, bool &$gzip): ISimpleFile { $encoding = $this->request->getHeader('Accept-Encoding'); diff --git a/core/Controller/OCJSController.php b/core/Controller/OCJSController.php index 083ad4b209f..db42a3608b5 100644 --- a/core/Controller/OCJSController.php +++ b/core/Controller/OCJSController.php @@ -16,6 +16,7 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoTwoFactorRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\DataDisplayResponse; @@ -75,12 +76,10 @@ class OCJSController extends Controller { ); } - /** - * @NoTwoFactorRequired - */ #[PublicPage] #[NoCSRFRequired] #[FrontpageRoute(verb: 'GET', url: '/core/js/oc.js')] + #[NoTwoFactorRequired] public function getConfig(): DataDisplayResponse { $data = $this->helper->getConfig(); diff --git a/core/Controller/TwoFactorChallengeController.php b/core/Controller/TwoFactorChallengeController.php index 8072bb8e92a..a10729e456d 100644 --- a/core/Controller/TwoFactorChallengeController.php +++ b/core/Controller/TwoFactorChallengeController.php @@ -7,6 +7,7 @@ */ namespace OC\Core\Controller; +use OC\AppFramework\Http\Attributes\TwoFactorSetUpDoneRequired; use OC\Authentication\TwoFactorAuth\Manager; use OC_User; use OCP\AppFramework\Controller; @@ -67,16 +68,11 @@ class TwoFactorChallengeController extends Controller { return [$regular, $backup]; } - /** - * @TwoFactorSetUpDoneRequired - * - * @param string $redirect_url - * @return StandaloneTemplateResponse - */ #[NoAdminRequired] #[NoCSRFRequired] #[FrontpageRoute(verb: 'GET', url: '/login/selectchallenge')] - public function selectChallenge($redirect_url) { + #[TwoFactorSetUpDoneRequired] + public function selectChallenge(string $redirect_url): StandaloneTemplateResponse { $user = $this->userSession->getUser(); $providerSet = $this->twoFactorManager->getProviderSet($user); $allProviders = $providerSet->getProviders(); @@ -95,18 +91,12 @@ class TwoFactorChallengeController extends Controller { return new StandaloneTemplateResponse($this->appName, 'twofactorselectchallenge', $data, 'guest'); } - /** - * @TwoFactorSetUpDoneRequired - * - * @param string $challengeProviderId - * @param string $redirect_url - * @return StandaloneTemplateResponse|RedirectResponse - */ #[NoAdminRequired] #[NoCSRFRequired] #[UseSession] + #[TwoFactorSetUpDoneRequired] #[FrontpageRoute(verb: 'GET', url: '/login/challenge/{challengeProviderId}')] - public function showChallenge($challengeProviderId, $redirect_url) { + public function showChallenge(string $challengeProviderId, string $redirect_url): StandaloneTemplateResponse|RedirectResponse { $user = $this->userSession->getUser(); $providerSet = $this->twoFactorManager->getProviderSet($user); $provider = $providerSet->getProvider($challengeProviderId); @@ -148,21 +138,13 @@ class TwoFactorChallengeController extends Controller { return $response; } - /** - * @TwoFactorSetUpDoneRequired - * - * - * @param string $challengeProviderId - * @param string $challenge - * @param string $redirect_url - * @return RedirectResponse - */ #[NoAdminRequired] #[NoCSRFRequired] #[UseSession] #[FrontpageRoute(verb: 'POST', url: '/login/challenge/{challengeProviderId}')] + #[TwoFactorSetUpDoneRequired] #[UserRateLimit(limit: 5, period: 100)] - public function solveChallenge($challengeProviderId, $challenge, $redirect_url = null) { + public function solveChallenge(string $challengeProviderId, string $challenge, ?string $redirect_url = null): RedirectResponse { $user = $this->userSession->getUser(); $provider = $this->twoFactorManager->getProvider($user, $challengeProviderId); if (is_null($provider)) { diff --git a/core/Middleware/TwoFactorMiddleware.php b/core/Middleware/TwoFactorMiddleware.php index 0dea402f127..afc693c15bb 100644 --- a/core/Middleware/TwoFactorMiddleware.php +++ b/core/Middleware/TwoFactorMiddleware.php @@ -10,6 +10,7 @@ declare(strict_types=1); namespace OC\Core\Middleware; use Exception; +use OC\AppFramework\Http\Attributes\TwoFactorSetUpDoneRequired; use OC\Authentication\Exceptions\TwoFactorAuthRequiredException; use OC\Authentication\Exceptions\UserAlreadyLoggedInException; use OC\Authentication\TwoFactorAuth\Manager; @@ -18,6 +19,7 @@ use OC\Core\Controller\TwoFactorChallengeController; use OC\User\Session; use OCA\TwoFactorNextcloudNotification\Controller\APIController; use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\Attribute\NoTwoFactorRequired; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Middleware; use OCP\AppFramework\Utility\IControllerMethodReflector; @@ -26,6 +28,8 @@ use OCP\IRequest; use OCP\ISession; use OCP\IURLGenerator; use OCP\IUser; +use Psr\Log\LoggerInterface; +use ReflectionMethod; class TwoFactorMiddleware extends Middleware { public function __construct( @@ -35,6 +39,7 @@ class TwoFactorMiddleware extends Middleware { private IURLGenerator $urlGenerator, private IControllerMethodReflector $reflector, private IRequest $request, + private LoggerInterface $logger, ) { } @@ -43,7 +48,9 @@ class TwoFactorMiddleware extends Middleware { * @param string $methodName */ public function beforeController($controller, $methodName) { - if ($this->reflector->hasAnnotation('NoTwoFactorRequired')) { + $reflectionMethod = new ReflectionMethod($controller, $methodName); + + if ($this->hasAnnotationOrAttribute($reflectionMethod, 'NoTwoFactorRequired', NoTwoFactorRequired::class)) { // Route handler explicitly marked to work without finished 2FA are // not blocked return; @@ -56,7 +63,7 @@ class TwoFactorMiddleware extends Middleware { if ($controller instanceof TwoFactorChallengeController && $this->userSession->getUser() !== null - && !$this->reflector->hasAnnotation('TwoFactorSetUpDoneRequired')) { + && !$reflectionMethod->getAttributes(TwoFactorSetUpDoneRequired::class)) { $providers = $this->twoFactorManager->getProviderSet($this->userSession->getUser()); if (!($providers->getPrimaryProviders() === [] && !$providers->isProviderMissing())) { @@ -86,7 +93,7 @@ class TwoFactorMiddleware extends Middleware { || $this->session->exists('app_api') // authenticated using an AppAPI Auth || $this->twoFactorManager->isTwoFactorAuthenticated($user)) { - $this->checkTwoFactor($controller, $methodName, $user); + $this->checkTwoFactor($controller, $user); } elseif ($controller instanceof TwoFactorChallengeController) { // Allow access to the two-factor controllers only if two-factor authentication // is in progress. @@ -96,7 +103,7 @@ class TwoFactorMiddleware extends Middleware { // TODO: dont check/enforce 2FA if a auth token is used } - private function checkTwoFactor(Controller $controller, $methodName, IUser $user) { + private function checkTwoFactor(Controller $controller, IUser $user) { // If two-factor auth is in progress disallow access to any controllers // defined within "LoginController". $needsSecondFactor = $this->twoFactorManager->needsSecondFactor($user); @@ -130,4 +137,26 @@ class TwoFactorMiddleware extends Middleware { throw $exception; } + + + /** + * @template T + * + * @param ReflectionMethod $reflectionMethod + * @param ?string $annotationName + * @param class-string $attributeClass + * @return boolean + */ + protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, ?string $annotationName, string $attributeClass): bool { + if (!empty($reflectionMethod->getAttributes($attributeClass))) { + return true; + } + + if ($annotationName && $this->reflector->hasAnnotation($annotationName)) { + $this->logger->debug($reflectionMethod->getDeclaringClass()->getName() . '::' . $reflectionMethod->getName() . ' uses the @' . $annotationName . ' annotation and should use the #[' . $attributeClass . '] attribute instead'); + return true; + } + + return false; + } } diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 7feba98285a..165c7c05cfd 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -93,6 +93,7 @@ return array( 'OCP\\AppFramework\\Http\\Attribute\\IgnoreOpenAPI' => $baseDir . '/lib/public/AppFramework/Http/Attribute/IgnoreOpenAPI.php', 'OCP\\AppFramework\\Http\\Attribute\\NoAdminRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoAdminRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\NoCSRFRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoCSRFRequired.php', + 'OCP\\AppFramework\\Http\\Attribute\\NoTwoFactorRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoTwoFactorRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\OpenAPI' => $baseDir . '/lib/public/AppFramework/Http/Attribute/OpenAPI.php', 'OCP\\AppFramework\\Http\\Attribute\\PasswordConfirmationRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/PasswordConfirmationRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\PublicPage' => $baseDir . '/lib/public/AppFramework/Http/Attribute/PublicPage.php', @@ -1092,6 +1093,7 @@ return array( 'OC\\AppFramework\\Bootstrap\\ServiceRegistration' => $baseDir . '/lib/private/AppFramework/Bootstrap/ServiceRegistration.php', 'OC\\AppFramework\\DependencyInjection\\DIContainer' => $baseDir . '/lib/private/AppFramework/DependencyInjection/DIContainer.php', 'OC\\AppFramework\\Http' => $baseDir . '/lib/private/AppFramework/Http.php', + 'OC\\AppFramework\\Http\\Attributes\\TwoFactorSetUpDoneRequired' => $baseDir . '/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php', 'OC\\AppFramework\\Http\\Dispatcher' => $baseDir . '/lib/private/AppFramework/Http/Dispatcher.php', 'OC\\AppFramework\\Http\\Output' => $baseDir . '/lib/private/AppFramework/Http/Output.php', 'OC\\AppFramework\\Http\\Request' => $baseDir . '/lib/private/AppFramework/Http/Request.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 772c36f6bf2..4dbfb44cb8c 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -134,6 +134,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\AppFramework\\Http\\Attribute\\IgnoreOpenAPI' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/IgnoreOpenAPI.php', 'OCP\\AppFramework\\Http\\Attribute\\NoAdminRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoAdminRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\NoCSRFRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoCSRFRequired.php', + 'OCP\\AppFramework\\Http\\Attribute\\NoTwoFactorRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoTwoFactorRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\OpenAPI' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/OpenAPI.php', 'OCP\\AppFramework\\Http\\Attribute\\PasswordConfirmationRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/PasswordConfirmationRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\PublicPage' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/PublicPage.php', @@ -1133,6 +1134,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\AppFramework\\Bootstrap\\ServiceRegistration' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Bootstrap/ServiceRegistration.php', 'OC\\AppFramework\\DependencyInjection\\DIContainer' => __DIR__ . '/../../..' . '/lib/private/AppFramework/DependencyInjection/DIContainer.php', 'OC\\AppFramework\\Http' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http.php', + 'OC\\AppFramework\\Http\\Attributes\\TwoFactorSetUpDoneRequired' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php', 'OC\\AppFramework\\Http\\Dispatcher' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Dispatcher.php', 'OC\\AppFramework\\Http\\Output' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Output.php', 'OC\\AppFramework\\Http\\Request' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Http/Request.php', diff --git a/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php b/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php new file mode 100644 index 00000000000..f298fff77eb --- /dev/null +++ b/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php @@ -0,0 +1,18 @@ +session = $this->createMock(ISession::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->reflector = $this->createMock(IControllerMethodReflector::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->request = new Request( [ 'server' => [ @@ -78,8 +101,7 @@ class TwoFactorMiddlewareTest extends TestCase { $this->createMock(IConfig::class) ); - $this->middleware = new TwoFactorMiddleware($this->twoFactorManager, $this->userSession, $this->session, $this->urlGenerator, $this->reflector, $this->request); - $this->controller = $this->createMock(Controller::class); + $this->middleware = new TwoFactorMiddleware($this->twoFactorManager, $this->userSession, $this->session, $this->urlGenerator, $this->reflector, $this->request, $this->logger); } public function testBeforeControllerNotLoggedIn(): void { @@ -90,12 +112,14 @@ class TwoFactorMiddlewareTest extends TestCase { $this->userSession->expects($this->never()) ->method('getUser'); - $this->middleware->beforeController($this->controller, 'index'); + $controller = $this->getMockBuilder(NoTwoFactorAnnotationController::class) + ->disableOriginalConstructor() + ->getMock(); + $this->middleware->beforeController($controller, 'index'); } public function testBeforeSetupController(): void { $user = $this->createMock(IUser::class); - $controller = $this->createMock(ALoginSetupController::class); $this->userSession->expects($this->any()) ->method('getUser') ->willReturn($user); @@ -105,7 +129,7 @@ class TwoFactorMiddlewareTest extends TestCase { $this->userSession->expects($this->never()) ->method('isLoggedIn'); - $this->middleware->beforeController($controller, 'create'); + $this->middleware->beforeController(new LoginSetupController('foo', $this->request), 'index'); } public function testBeforeControllerNoTwoFactorCheckNeeded(): void { @@ -122,7 +146,10 @@ class TwoFactorMiddlewareTest extends TestCase { ->with($user) ->willReturn(false); - $this->middleware->beforeController($this->controller, 'index'); + $controller = $this->getMockBuilder(NoTwoFactorAnnotationController::class) + ->disableOriginalConstructor() + ->getMock(); + $this->middleware->beforeController($controller, 'index'); } @@ -146,7 +173,10 @@ class TwoFactorMiddlewareTest extends TestCase { ->with($user) ->willReturn(true); - $this->middleware->beforeController($this->controller, 'index'); + $controller = $this->getMockBuilder(NoTwoFactorAnnotationController::class) + ->disableOriginalConstructor() + ->getMock(); + $this->middleware->beforeController($controller, 'index'); } @@ -155,9 +185,6 @@ class TwoFactorMiddlewareTest extends TestCase { $user = $this->createMock(IUser::class); - $this->reflector - ->method('hasAnnotation') - ->willReturn(false); $this->userSession->expects($this->once()) ->method('isLoggedIn') ->willReturn(true); @@ -173,7 +200,7 @@ class TwoFactorMiddlewareTest extends TestCase { ->with($user) ->willReturn(false); - $twoFactorChallengeController = $this->getMockBuilder(TwoFactorChallengeController::class) + $twoFactorChallengeController = $this->getMockBuilder(NoTwoFactorChallengeAnnotationController::class) ->disableOriginalConstructor() ->getMock(); $this->middleware->beforeController($twoFactorChallengeController, 'index'); @@ -188,7 +215,8 @@ class TwoFactorMiddlewareTest extends TestCase { ->willReturn('test/url'); $expected = new RedirectResponse('test/url'); - $this->assertEquals($expected, $this->middleware->afterException($this->controller, 'index', $ex)); + $controller = new HasTwoFactorAnnotationController('foo', $this->request); + $this->assertEquals($expected, $this->middleware->afterException($controller, 'index', $ex)); } public function testAfterException(): void { @@ -200,17 +228,13 @@ class TwoFactorMiddlewareTest extends TestCase { ->willReturn('redirect/url'); $expected = new RedirectResponse('redirect/url'); - $this->assertEquals($expected, $this->middleware->afterException($this->controller, 'index', $ex)); + $controller = new HasTwoFactorAnnotationController('foo', $this->request); + $this->assertEquals($expected, $this->middleware->afterException($controller, 'index', $ex)); } public function testRequires2FASetupDoneAnnotated(): void { $user = $this->createMock(IUser::class); - $this->reflector - ->method('hasAnnotation') - ->willReturnCallback(function (string $annotation) { - return $annotation === 'TwoFactorSetUpDoneRequired'; - }); $this->userSession->expects($this->once()) ->method('isLoggedIn') ->willReturn(true); @@ -228,10 +252,10 @@ class TwoFactorMiddlewareTest extends TestCase { $this->expectException(UserAlreadyLoggedInException::class); - $twoFactorChallengeController = $this->getMockBuilder(TwoFactorChallengeController::class) + $controller = $this->getMockBuilder(HasTwoFactorSetUpDoneAnnotationController::class) ->disableOriginalConstructor() ->getMock(); - $this->middleware->beforeController($twoFactorChallengeController, 'index'); + $this->middleware->beforeController($controller, 'index'); } public static function dataRequires2FASetupDone(): array { @@ -243,7 +267,7 @@ class TwoFactorMiddlewareTest extends TestCase { ]; } - #[\PHPUnit\Framework\Attributes\DataProvider('dataRequires2FASetupDone')] + #[DataProvider('dataRequires2FASetupDone')] public function testRequires2FASetupDone(bool $hasProvider, bool $missingProviders, bool $expectEception): void { if ($hasProvider) { $provider = $this->createMock(IProvider::class); @@ -257,9 +281,6 @@ class TwoFactorMiddlewareTest extends TestCase { $user = $this->createMock(IUser::class); - $this->reflector - ->method('hasAnnotation') - ->willReturn(false); $this->userSession ->method('getUser') ->willReturn($user); @@ -278,9 +299,9 @@ class TwoFactorMiddlewareTest extends TestCase { $this->assertTrue(true); } - $twoFactorChallengeController = $this->getMockBuilder(TwoFactorChallengeController::class) + $controller = $this->getMockBuilder(NoTwoFactorChallengeAnnotationController::class) ->disableOriginalConstructor() ->getMock(); - $this->middleware->beforeController($twoFactorChallengeController, 'index'); + $this->middleware->beforeController($controller, 'index'); } } From 6408ed0b51532b5d9c7a8c2664e8d8d88173d6b2 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Wed, 17 Sep 2025 15:54:17 +0200 Subject: [PATCH 2/4] feat(AppFramework): Add missing NoSameSiteCookieRequired attribute Allow to replace the old annotation. Signed-off-by: Carl Schwan --- .../Controller/PublicPreviewController.php | 4 +- .../lib/Controller/ShareController.php | 11 ++--- .../lib/Controller/ThemingController.php | 4 +- core/Controller/AvatarController.php | 7 ++- core/Controller/ClientFlowLoginController.php | 5 +- .../ClientFlowLoginV2Controller.php | 5 +- core/Controller/CssController.php | 10 ++-- core/Controller/JsController.php | 5 +- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../Attributes/TwoFactorSetUpDoneRequired.php | 2 +- .../Security/SameSiteCookieMiddleware.php | 34 ++++++++++++-- .../Attribute/NoSameSiteCookieRequired.php | 25 ++++++++++ .../Http/Attribute/NoTwoFactorRequired.php | 7 ++- .../Security/SameSiteCookieMiddlewareTest.php | 46 ++++++++++++------- 15 files changed, 115 insertions(+), 52 deletions(-) create mode 100644 lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php diff --git a/apps/files_sharing/lib/Controller/PublicPreviewController.php b/apps/files_sharing/lib/Controller/PublicPreviewController.php index d917f6e0ebb..9056161af84 100644 --- a/apps/files_sharing/lib/Controller/PublicPreviewController.php +++ b/apps/files_sharing/lib/Controller/PublicPreviewController.php @@ -8,6 +8,7 @@ namespace OCA\Files_Sharing\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\DataResponse; @@ -144,8 +145,6 @@ class PublicPreviewController extends PublicShareController { } /** - * @NoSameSiteCookieRequired - * * Get a direct link preview for a shared file * * @param string $token Token of the share @@ -159,6 +158,7 @@ class PublicPreviewController extends PublicShareController { #[PublicPage] #[NoCSRFRequired] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] + #[NoSameSiteCookieRequired] public function directLink(string $token) { // No token no image if ($token === '') { diff --git a/apps/files_sharing/lib/Controller/ShareController.php b/apps/files_sharing/lib/Controller/ShareController.php index 3c51a95f1d5..ab7556e1af9 100644 --- a/apps/files_sharing/lib/Controller/ShareController.php +++ b/apps/files_sharing/lib/Controller/ShareController.php @@ -15,12 +15,12 @@ use OCA\Files_Sharing\Event\ShareLinkAccessedEvent; use OCP\Accounts\IAccountManager; use OCP\AppFramework\AuthPublicShareController; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\NotFoundResponse; use OCP\AppFramework\Http\RedirectResponse; -use OCP\AppFramework\Http\Response; use OCP\AppFramework\Http\TemplateResponse; use OCP\Constants; use OCP\Defaults; @@ -343,18 +343,13 @@ class ShareController extends AuthPublicShareController { } /** - * @NoSameSiteCookieRequired - * - * @param string $token - * @param string|null $files - * @param string $path - * @return void|Response * @throws NotFoundException * @deprecated 31.0.0 Users are encouraged to use the DAV endpoint */ #[PublicPage] #[NoCSRFRequired] - public function downloadShare($token, $files = null, $path = '') { + #[NoSameSiteCookieRequired] + public function downloadShare(string $token, ?string $files = null, string $path = ''): NotFoundResponse|RedirectResponse|DataResponse { \OC_User::setIncognitoMode(true); $share = $this->shareManager->getShareByToken($token); diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php index a581605c374..467caef40a0 100644 --- a/apps/theming/lib/Controller/ThemingController.php +++ b/apps/theming/lib/Controller/ThemingController.php @@ -17,6 +17,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\AuthorizedAdminSetting; use OCP\AppFramework\Http\Attribute\BruteForceProtection; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\NoTwoFactorRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; @@ -377,8 +378,6 @@ class ThemingController extends Controller { } /** - * @NoSameSiteCookieRequired - * * Get the CSS stylesheet for a theme * * @param string $themeId ID of the theme @@ -393,6 +392,7 @@ class ThemingController extends Controller { #[NoCSRFRequired] #[NoTwoFactorRequired] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] + #[NoSameSiteCookieRequired] public function getThemeStylesheet(string $themeId, bool $plain = false, bool $withCustomCss = false) { $themes = $this->themesService->getThemes(); if (!in_array($themeId, array_keys($themes))) { diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php index 1413c9d678e..3d6f79151e8 100644 --- a/core/Controller/AvatarController.php +++ b/core/Controller/AvatarController.php @@ -13,6 +13,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\FileDisplayResponse; @@ -50,8 +51,6 @@ class AvatarController extends Controller { } /** - * @NoSameSiteCookieRequired - * * Get the dark avatar * * @param string $userId ID of the user @@ -67,6 +66,7 @@ class AvatarController extends Controller { #[PublicPage] #[FrontpageRoute(verb: 'GET', url: '/avatar/{userId}/{size}/dark')] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] + #[NoSameSiteCookieRequired] public function getAvatarDark(string $userId, int $size, bool $guestFallback = false) { if ($size <= 64) { if ($size !== 64) { @@ -102,8 +102,6 @@ class AvatarController extends Controller { /** - * @NoSameSiteCookieRequired - * * Get the avatar * * @param string $userId ID of the user @@ -119,6 +117,7 @@ class AvatarController extends Controller { #[PublicPage] #[FrontpageRoute(verb: 'GET', url: '/avatar/{userId}/{size}')] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] + #[NoSameSiteCookieRequired] public function getAvatar(string $userId, int $size, bool $guestFallback = false) { if ($size <= 64) { if ($size !== 64) { diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 0e83ef283a9..836bba6d8b7 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -17,6 +17,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\Attribute\PublicPage; @@ -171,13 +172,11 @@ class ClientFlowLoginController extends Controller { return $response; } - /** - * @NoSameSiteCookieRequired - */ #[NoAdminRequired] #[NoCSRFRequired] #[UseSession] #[FrontpageRoute(verb: 'GET', url: '/login/flow/grant')] + #[NoSameSiteCookieRequired] public function grantPage( string $stateToken = '', string $clientIdentifier = '', diff --git a/core/Controller/ClientFlowLoginV2Controller.php b/core/Controller/ClientFlowLoginV2Controller.php index 9a1694a93be..85b82b388f4 100644 --- a/core/Controller/ClientFlowLoginV2Controller.php +++ b/core/Controller/ClientFlowLoginV2Controller.php @@ -18,6 +18,7 @@ use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Http\Attribute\PublicPage; @@ -142,14 +143,12 @@ class ClientFlowLoginV2Controller extends Controller { ); } - /** - * @NoSameSiteCookieRequired - */ #[NoAdminRequired] #[NoCSRFRequired] #[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)] #[UseSession] #[FrontpageRoute(verb: 'GET', url: '/login/v2/grant')] + #[NoSameSiteCookieRequired] public function grantPage(?string $stateToken, int $direct = 0): StandaloneTemplateResponse { if ($stateToken === null) { return $this->stateTokenMissingResponse(); diff --git a/core/Controller/CssController.php b/core/Controller/CssController.php index 37e7edc530f..b6b0777fe40 100644 --- a/core/Controller/CssController.php +++ b/core/Controller/CssController.php @@ -13,11 +13,11 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\FileDisplayResponse; use OCP\AppFramework\Http\NotFoundResponse; -use OCP\AppFramework\Http\Response; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\IAppData; use OCP\Files\NotFoundException; @@ -41,21 +41,19 @@ class CssController extends Controller { } /** - * @NoSameSiteCookieRequired - * * @param string $fileName css filename with extension * @param string $appName css folder name - * @return FileDisplayResponse|NotFoundResponse */ #[PublicPage] #[NoCSRFRequired] #[FrontpageRoute(verb: 'GET', url: '/css/{appName}/{fileName}')] - public function getCss(string $fileName, string $appName): Response { + #[NoSameSiteCookieRequired] + public function getCss(string $fileName, string $appName): FileDisplayResponse|NotFoundResponse { try { $folder = $this->appData->getFolder($appName); $gzip = false; $file = $this->getFile($folder, $fileName, $gzip); - } catch (NotFoundException $e) { + } catch (NotFoundException) { return new NotFoundResponse(); } diff --git a/core/Controller/JsController.php b/core/Controller/JsController.php index b74e0583683..f214d3043c2 100644 --- a/core/Controller/JsController.php +++ b/core/Controller/JsController.php @@ -13,12 +13,12 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; +use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Attribute\NoTwoFactorRequired; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\Attribute\PublicPage; use OCP\AppFramework\Http\FileDisplayResponse; use OCP\AppFramework\Http\NotFoundResponse; -use OCP\AppFramework\Http\Response; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\IAppData; use OCP\Files\NotFoundException; @@ -42,8 +42,6 @@ class JsController extends Controller { } /** - * @NoSameSiteCookieRequired - * * @param string $fileName js filename with extension * @param string $appName js folder name */ @@ -51,6 +49,7 @@ class JsController extends Controller { #[NoCSRFRequired] #[FrontpageRoute(verb: 'GET', url: '/js/{appName}/{fileName}')] #[NoTwoFactorRequired] + #[NoSameSiteCookieRequired] public function getJs(string $fileName, string $appName): FileDisplayResponse|NotFoundResponse { try { $folder = $this->appData->getFolder($appName); diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 165c7c05cfd..ba20ae33098 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -93,6 +93,7 @@ return array( 'OCP\\AppFramework\\Http\\Attribute\\IgnoreOpenAPI' => $baseDir . '/lib/public/AppFramework/Http/Attribute/IgnoreOpenAPI.php', 'OCP\\AppFramework\\Http\\Attribute\\NoAdminRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoAdminRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\NoCSRFRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoCSRFRequired.php', + 'OCP\\AppFramework\\Http\\Attribute\\NoSameSiteCookieRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\NoTwoFactorRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/NoTwoFactorRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\OpenAPI' => $baseDir . '/lib/public/AppFramework/Http/Attribute/OpenAPI.php', 'OCP\\AppFramework\\Http\\Attribute\\PasswordConfirmationRequired' => $baseDir . '/lib/public/AppFramework/Http/Attribute/PasswordConfirmationRequired.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 4dbfb44cb8c..3fb5e222e25 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -134,6 +134,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\AppFramework\\Http\\Attribute\\IgnoreOpenAPI' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/IgnoreOpenAPI.php', 'OCP\\AppFramework\\Http\\Attribute\\NoAdminRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoAdminRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\NoCSRFRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoCSRFRequired.php', + 'OCP\\AppFramework\\Http\\Attribute\\NoSameSiteCookieRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\NoTwoFactorRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/NoTwoFactorRequired.php', 'OCP\\AppFramework\\Http\\Attribute\\OpenAPI' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/OpenAPI.php', 'OCP\\AppFramework\\Http\\Attribute\\PasswordConfirmationRequired' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/PasswordConfirmationRequired.php', diff --git a/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php b/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php index f298fff77eb..377d4b959ab 100644 --- a/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php +++ b/lib/private/AppFramework/Http/Attributes/TwoFactorSetUpDoneRequired.php @@ -12,7 +12,7 @@ namespace OC\AppFramework\Http\Attributes; use Attribute; -#[Attribute] +#[Attribute(Attribute::TARGET_METHOD)] class TwoFactorSetUpDoneRequired { } diff --git a/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php b/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php index 097ed1b2b8f..3bc733931d4 100644 --- a/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php @@ -1,5 +1,7 @@ reflector->hasAnnotation('NoSameSiteCookieRequired'); + $reflectionMethod = new ReflectionMethod($controller, $methodName); + $noSSC = $this->hasAnnotationOrAttribute($reflectionMethod, 'NoSameSiteCookieRequired', NoSameSiteCookieRequired::class); if ($noSSC) { return; } @@ -80,4 +87,25 @@ class SameSiteCookieMiddleware extends Middleware { ); } } + + /** + * @template T + * + * @param ReflectionMethod $reflectionMethod + * @param ?string $annotationName + * @param class-string $attributeClass + * @return boolean + */ + protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, ?string $annotationName, string $attributeClass): bool { + if (!empty($reflectionMethod->getAttributes($attributeClass))) { + return true; + } + + if ($annotationName && $this->reflector->hasAnnotation($annotationName)) { + $this->logger->debug($reflectionMethod->getDeclaringClass()->getName() . '::' . $reflectionMethod->getName() . ' uses the @' . $annotationName . ' annotation and should use the #[' . $attributeClass . '] attribute instead'); + return true; + } + + return false; + } } diff --git a/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php b/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php new file mode 100644 index 00000000000..5bbb65cbe29 --- /dev/null +++ b/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php @@ -0,0 +1,25 @@ +request = $this->createMock(Request::class); $this->reflector = $this->createMock(ControllerMethodReflector::class); - $this->middleware = new SameSiteCookieMiddleware($this->request, $this->reflector); + $this->logger = $this->createMock(LoggerInterface::class); + $this->middleware = new SameSiteCookieMiddleware($this->request, $this->reflector, $this->logger); } public function testBeforeControllerNoIndex(): void { $this->request->method('getScriptName') ->willReturn('/ocs/v2.php'); - $this->middleware->beforeController($this->createMock(Controller::class), 'foo'); + $this->middleware->beforeController(new NoAnnotationController('foo', $this->request), 'foo'); $this->addToAssertionCount(1); } @@ -50,7 +64,7 @@ class SameSiteCookieMiddlewareTest extends TestCase { ->with('NoSameSiteCookieRequired') ->willReturn(true); - $this->middleware->beforeController($this->createMock(Controller::class), 'foo'); + $this->middleware->beforeController(new HasAnnotationController('foo', $this->request), 'foo'); $this->addToAssertionCount(1); } @@ -65,7 +79,7 @@ class SameSiteCookieMiddlewareTest extends TestCase { $this->request->method('passesLaxCookieCheck') ->willReturn(true); - $this->middleware->beforeController($this->createMock(Controller::class), 'foo'); + $this->middleware->beforeController(new NoAnnotationController('foo', $this->request), 'foo'); $this->addToAssertionCount(1); } @@ -82,14 +96,14 @@ class SameSiteCookieMiddlewareTest extends TestCase { $this->request->method('passesLaxCookieCheck') ->willReturn(false); - $this->middleware->beforeController($this->createMock(Controller::class), 'foo'); + $this->middleware->beforeController(new NoAnnotationController('foo', $this->request), 'foo'); } public function testAfterExceptionNoLaxCookie(): void { $ex = new SecurityException(); try { - $this->middleware->afterException($this->createMock(Controller::class), 'foo', $ex); + $this->middleware->afterException(new NoAnnotationController('foo', $this->request), 'foo', $ex); $this->fail(); } catch (\Exception $e) { $this->assertSame($ex, $e); @@ -103,14 +117,14 @@ class SameSiteCookieMiddlewareTest extends TestCase { ->willReturn('/myrequri'); $middleware = $this->getMockBuilder(SameSiteCookieMiddleware::class) - ->setConstructorArgs([$this->request, $this->reflector]) + ->setConstructorArgs([$this->request, $this->reflector, $this->logger]) ->onlyMethods(['setSameSiteCookie']) ->getMock(); $middleware->expects($this->once()) ->method('setSameSiteCookie'); - $resp = $middleware->afterException($this->createMock(Controller::class), 'foo', $ex); + $resp = $middleware->afterException(new NoAnnotationController('foo', $this->request), 'foo', $ex); $this->assertSame(Http::STATUS_FOUND, $resp->getStatus()); From f81475445d6d4a41c105b8bad4e80ed7b205c003 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 12 Jan 2026 13:15:47 +0100 Subject: [PATCH 3/4] refactor: Move hasAnnotationOrAttribute to MiddlewareUtils Signed-off-by: Carl Schwan --- build/psalm-baseline.xml | 7 -- core/Middleware/TwoFactorMiddleware.php | 30 +---- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../DependencyInjection/DIContainer.php | 7 +- lib/private/AppFramework/Http/Dispatcher.php | 91 ++++------------ .../Middleware/MiddlewareUtils.php | 70 ++++++++++++ .../Middleware/Security/CORSMiddleware.php | 90 +++------------ .../Security/SameSiteCookieMiddleware.php | 29 +---- .../Security/SecurityMiddleware.php | 103 +++++------------- .../Middleware/TwoFactorMiddlewareTest.php | 9 +- .../Security/CORSMiddlewareTest.php | 37 +++---- .../Security/SameSiteCookieMiddlewareTest.php | 7 +- .../Security/SecurityMiddlewareTest.php | 45 +++----- 14 files changed, 194 insertions(+), 333 deletions(-) create mode 100644 lib/private/AppFramework/Middleware/MiddlewareUtils.php diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 4b141d200f1..748f3b3b4db 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -3210,13 +3210,6 @@ - - - - - - - request->server]]> diff --git a/core/Middleware/TwoFactorMiddleware.php b/core/Middleware/TwoFactorMiddleware.php index afc693c15bb..02f8ab8dad3 100644 --- a/core/Middleware/TwoFactorMiddleware.php +++ b/core/Middleware/TwoFactorMiddleware.php @@ -11,6 +11,7 @@ namespace OC\Core\Middleware; use Exception; use OC\AppFramework\Http\Attributes\TwoFactorSetUpDoneRequired; +use OC\AppFramework\Middleware\MiddlewareUtils; use OC\Authentication\Exceptions\TwoFactorAuthRequiredException; use OC\Authentication\Exceptions\UserAlreadyLoggedInException; use OC\Authentication\TwoFactorAuth\Manager; @@ -22,13 +23,11 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\NoTwoFactorRequired; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Middleware; -use OCP\AppFramework\Utility\IControllerMethodReflector; use OCP\Authentication\TwoFactorAuth\ALoginSetupController; use OCP\IRequest; use OCP\ISession; use OCP\IURLGenerator; use OCP\IUser; -use Psr\Log\LoggerInterface; use ReflectionMethod; class TwoFactorMiddleware extends Middleware { @@ -37,9 +36,8 @@ class TwoFactorMiddleware extends Middleware { private Session $userSession, private ISession $session, private IURLGenerator $urlGenerator, - private IControllerMethodReflector $reflector, + private MiddlewareUtils $middlewareUtils, private IRequest $request, - private LoggerInterface $logger, ) { } @@ -50,7 +48,7 @@ class TwoFactorMiddleware extends Middleware { public function beforeController($controller, $methodName) { $reflectionMethod = new ReflectionMethod($controller, $methodName); - if ($this->hasAnnotationOrAttribute($reflectionMethod, 'NoTwoFactorRequired', NoTwoFactorRequired::class)) { + if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoTwoFactorRequired', NoTwoFactorRequired::class)) { // Route handler explicitly marked to work without finished 2FA are // not blocked return; @@ -137,26 +135,4 @@ class TwoFactorMiddleware extends Middleware { throw $exception; } - - - /** - * @template T - * - * @param ReflectionMethod $reflectionMethod - * @param ?string $annotationName - * @param class-string $attributeClass - * @return boolean - */ - protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, ?string $annotationName, string $attributeClass): bool { - if (!empty($reflectionMethod->getAttributes($attributeClass))) { - return true; - } - - if ($annotationName && $this->reflector->hasAnnotation($annotationName)) { - $this->logger->debug($reflectionMethod->getDeclaringClass()->getName() . '::' . $reflectionMethod->getName() . ' uses the @' . $annotationName . ' annotation and should use the #[' . $attributeClass . '] attribute instead'); - return true; - } - - return false; - } } diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index ba20ae33098..09e3f60b2f5 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1103,6 +1103,7 @@ return array( 'OC\\AppFramework\\Middleware\\CompressionMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/CompressionMiddleware.php', 'OC\\AppFramework\\Middleware\\FlowV2EphemeralSessionsMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/FlowV2EphemeralSessionsMiddleware.php', 'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => $baseDir . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php', + 'OC\\AppFramework\\Middleware\\MiddlewareUtils' => $baseDir . '/lib/private/AppFramework/Middleware/MiddlewareUtils.php', 'OC\\AppFramework\\Middleware\\NotModifiedMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php', 'OC\\AppFramework\\Middleware\\OCSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/OCSMiddleware.php', 'OC\\AppFramework\\Middleware\\PublicShare\\Exceptions\\NeedAuthenticationException' => $baseDir . '/lib/private/AppFramework/Middleware/PublicShare/Exceptions/NeedAuthenticationException.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 3fb5e222e25..1577d6ac781 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1144,6 +1144,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\AppFramework\\Middleware\\CompressionMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/CompressionMiddleware.php', 'OC\\AppFramework\\Middleware\\FlowV2EphemeralSessionsMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/FlowV2EphemeralSessionsMiddleware.php', 'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php', + 'OC\\AppFramework\\Middleware\\MiddlewareUtils' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/MiddlewareUtils.php', 'OC\\AppFramework\\Middleware\\NotModifiedMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php', 'OC\\AppFramework\\Middleware\\OCSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/OCSMiddleware.php', 'OC\\AppFramework\\Middleware\\PublicShare\\Exceptions\\NeedAuthenticationException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/PublicShare/Exceptions/NeedAuthenticationException.php', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 78951d99364..82b840b8072 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -17,6 +17,7 @@ use OC\AppFramework\Middleware\AdditionalScriptsMiddleware; use OC\AppFramework\Middleware\CompressionMiddleware; use OC\AppFramework\Middleware\FlowV2EphemeralSessionsMiddleware; use OC\AppFramework\Middleware\MiddlewareDispatcher; +use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\NotModifiedMiddleware; use OC\AppFramework\Middleware\OCSMiddleware; use OC\AppFramework\Middleware\PublicShare\PublicShareMiddleware; @@ -31,6 +32,7 @@ use OC\AppFramework\Middleware\Security\SameSiteCookieMiddleware; use OC\AppFramework\Middleware\Security\SecurityMiddleware; use OC\AppFramework\Middleware\SessionMiddleware; use OC\AppFramework\ScopedPsrLogger; +use OC\AppFramework\Utility\ControllerMethodReflector; use OC\AppFramework\Utility\SimpleContainer; use OC\Core\Middleware\TwoFactorMiddleware; use OC\Diagnostics\EventLogger; @@ -44,7 +46,6 @@ use OCP\AppFramework\IAppContainer; use OCP\AppFramework\QueryException; use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Services\IInitialState; -use OCP\AppFramework\Utility\IControllerMethodReflector; use OCP\Files\AppData\IAppDataFactory; use OCP\Files\Folder; use OCP\Files\IAppData; @@ -155,7 +156,7 @@ class DIContainer extends SimpleContainer implements IAppContainer { return new Dispatcher( $c->get(Http::class), $c->get(MiddlewareDispatcher::class), - $c->get(IControllerMethodReflector::class), + $c->get(ControllerMethodReflector::class), $c->get(IRequest::class), $c->get(IConfig::class), $c->get(IDBConnection::class), @@ -193,7 +194,7 @@ class DIContainer extends SimpleContainer implements IAppContainer { $securityMiddleware = new SecurityMiddleware( $c->get(IRequest::class), - $c->get(IControllerMethodReflector::class), + $c->get(MiddlewareUtils::class), $c->get(INavigationManager::class), $c->get(IURLGenerator::class), $c->get(LoggerInterface::class), diff --git a/lib/private/AppFramework/Http/Dispatcher.php b/lib/private/AppFramework/Http/Dispatcher.php index 9d786992831..ba0306a3fa5 100644 --- a/lib/private/AppFramework/Http/Dispatcher.php +++ b/lib/private/AppFramework/Http/Dispatcher.php @@ -26,64 +26,22 @@ use Psr\Log\LoggerInterface; * Class to dispatch the request to the middleware dispatcher */ class Dispatcher { - /** @var MiddlewareDispatcher */ - private $middlewareDispatcher; - - /** @var Http */ - private $protocol; - - /** @var ControllerMethodReflector */ - private $reflector; - - /** @var IRequest */ - private $request; - - /** @var IConfig */ - private $config; - - /** @var ConnectionAdapter */ - private $connection; - - /** @var LoggerInterface */ - private $logger; - - /** @var IEventLogger */ - private $eventLogger; - - private ContainerInterface $appContainer; - /** * @param Http $protocol the http protocol with contains all status headers * @param MiddlewareDispatcher $middlewareDispatcher the dispatcher which * runs the middleware - * @param ControllerMethodReflector $reflector the reflector that is used to inject - * the arguments for the controller - * @param IRequest $request the incoming request - * @param IConfig $config - * @param ConnectionAdapter $connection - * @param LoggerInterface $logger - * @param IEventLogger $eventLogger */ public function __construct( - Http $protocol, - MiddlewareDispatcher $middlewareDispatcher, - ControllerMethodReflector $reflector, - IRequest $request, - IConfig $config, - ConnectionAdapter $connection, - LoggerInterface $logger, - IEventLogger $eventLogger, - ContainerInterface $appContainer, + private readonly Http $protocol, + private readonly MiddlewareDispatcher $middlewareDispatcher, + private readonly ControllerMethodReflector $reflector, + private readonly IRequest $request, + private readonly IConfig $config, + private readonly ConnectionAdapter $connection, + private readonly LoggerInterface $logger, + private readonly IEventLogger $eventLogger, + private readonly ContainerInterface $appContainer, ) { - $this->protocol = $protocol; - $this->middlewareDispatcher = $middlewareDispatcher; - $this->reflector = $reflector; - $this->request = $request; - $this->config = $config; - $this->connection = $connection; - $this->logger = $logger; - $this->eventLogger = $eventLogger; - $this->appContainer = $appContainer; } @@ -92,16 +50,15 @@ class Dispatcher { * @param Controller $controller the controller which will be called * @param string $methodName the method name which will be called on * the controller - * @return array $array[0] contains the http status header as a string, - * $array[1] contains response headers as an array, - * $array[2] contains response cookies as an array, - * $array[3] contains the response output as a string, - * $array[4] contains the response object + * @return array{0: string, 1: array, 2: array, 3: string, 4: Response} + * $array[0] contains the http status header as a string, + * $array[1] contains response headers as an array, + * $array[2] contains response cookies as an array, + * $array[3] contains the response output as a string, + * $array[4] contains the response object * @throws \Exception */ public function dispatch(Controller $controller, string $methodName): array { - $out = [null, [], null]; - try { // prefill reflector with everything that's needed for the // middlewares @@ -156,15 +113,15 @@ class Dispatcher { $controller, $methodName, $response); // depending on the cache object the headers need to be changed - $out[0] = $this->protocol->getStatusHeader($response->getStatus()); - $out[1] = array_merge($response->getHeaders()); - $out[2] = $response->getCookies(); - $out[3] = $this->middlewareDispatcher->beforeOutput( - $controller, $methodName, $response->render() - ); - $out[4] = $response; - - return $out; + return [ + $this->protocol->getStatusHeader($response->getStatus()), + array_merge($response->getHeaders()), + $response->getCookies(), + $this->middlewareDispatcher->beforeOutput( + $controller, $methodName, $response->render() + ), + $response, + ]; } diff --git a/lib/private/AppFramework/Middleware/MiddlewareUtils.php b/lib/private/AppFramework/Middleware/MiddlewareUtils.php new file mode 100644 index 00000000000..94ac2d1f44c --- /dev/null +++ b/lib/private/AppFramework/Middleware/MiddlewareUtils.php @@ -0,0 +1,70 @@ + $attributeClass + * @return boolean + */ + public function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, ?string $annotationName, string $attributeClass): bool { + if (!empty($reflectionMethod->getAttributes($attributeClass))) { + return true; + } + + if ($annotationName && $this->reflector->hasAnnotation($annotationName)) { + $this->logger->debug($reflectionMethod->getDeclaringClass()->getName() . '::' . $reflectionMethod->getName() . ' uses the @' . $annotationName . ' annotation and should use the #[' . $attributeClass . '] attribute instead'); + return true; + } + + return false; + } + + /** + * @param ReflectionMethod $reflectionMethod + * @return string[] + */ + public function getAuthorizedAdminSettingClasses(ReflectionMethod $reflectionMethod): array { + $classes = []; + if ($this->reflector->hasAnnotation('AuthorizedAdminSetting')) { + $classes = explode(';', $this->reflector->getAnnotationParameter('AuthorizedAdminSetting', 'settings')); + } + + $attributes = $reflectionMethod->getAttributes(AuthorizedAdminSetting::class); + if (!empty($attributes)) { + foreach ($attributes as $attribute) { + /** @var AuthorizedAdminSetting $setting */ + $setting = $attribute->newInstance(); + $classes[] = $setting->getSettings(); + } + } + + return $classes; + } +} diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index 4453f5a7d4b..2d87f616a9d 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -7,8 +7,8 @@ */ namespace OC\AppFramework\Middleware\Security; +use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; -use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Authentication\Exceptions\PasswordLoginForbiddenException; use OC\User\Session; use OCP\AppFramework\Controller; @@ -21,7 +21,7 @@ use OCP\AppFramework\Middleware; use OCP\IRequest; use OCP\ISession; use OCP\Security\Bruteforce\IThrottler; -use Psr\Log\LoggerInterface; +use Override; use ReflectionMethod; /** @@ -31,45 +31,23 @@ use ReflectionMethod; * https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS */ class CORSMiddleware extends Middleware { - /** @var IRequest */ - private $request; - /** @var ControllerMethodReflector */ - private $reflector; - /** @var Session */ - private $session; - /** @var IThrottler */ - private $throttler; public function __construct( - IRequest $request, - ControllerMethodReflector $reflector, - Session $session, - IThrottler $throttler, - private readonly LoggerInterface $logger, + private readonly IRequest $request, + private readonly MiddlewareUtils $middlewareUtils, + private readonly Session $session, + private readonly IThrottler $throttler, ) { - $this->request = $request; - $this->reflector = $reflector; - $this->session = $session; - $this->throttler = $throttler; } - /** - * This is being run in normal order before the controller is being - * called which allows several modifications and checks - * - * @param Controller $controller the controller that is being called - * @param string $methodName the name of the method that will be called on - * the controller - * @throws SecurityException - * @since 6.0.0 - */ - public function beforeController($controller, $methodName) { + #[Override] + public function beforeController(Controller $controller, string $methodName): void { $reflectionMethod = new ReflectionMethod($controller, $methodName); // ensure that @CORS annotated API routes are not used in conjunction // with session authentication since this enables CSRF attack vectors - if ($this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class) - && (!$this->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class) || $this->session->isLoggedIn())) { + if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class) + && (!$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class) || $this->session->isLoggedIn())) { $user = array_key_exists('PHP_AUTH_USER', $this->request->server) ? $this->request->server['PHP_AUTH_USER'] : null; $pass = array_key_exists('PHP_AUTH_PW', $this->request->server) ? $this->request->server['PHP_AUTH_PW'] : null; @@ -92,45 +70,13 @@ class CORSMiddleware extends Middleware { } } - /** - * @template T - * - * @param ReflectionMethod $reflectionMethod - * @param string $annotationName - * @param class-string $attributeClass - * @return boolean - */ - protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, string $annotationName, string $attributeClass): bool { - if ($this->reflector->hasAnnotation($annotationName)) { - $this->logger->debug($reflectionMethod->getDeclaringClass()->getName() . '::' . $reflectionMethod->getName() . ' uses the @' . $annotationName . ' annotation and should use the #[' . $attributeClass . '] attribute instead'); - return true; - } - - - if (!empty($reflectionMethod->getAttributes($attributeClass))) { - return true; - } - - return false; - } - - /** - * This is being run after a successful controller method call and allows - * the manipulation of a Response object. The middleware is run in reverse order - * - * @param Controller $controller the controller that is being called - * @param string $methodName the name of the method that will be called on - * the controller - * @param Response $response the generated response from the controller - * @return Response a Response object - * @throws SecurityException - */ - public function afterController($controller, $methodName, Response $response) { + #[Override] + public function afterController(Controller $controller, string $methodName, Response $response): Response { // only react if it's a CORS request and if the request sends origin and if (isset($this->request->server['HTTP_ORIGIN'])) { $reflectionMethod = new ReflectionMethod($controller, $methodName); - if ($this->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class)) { + if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'CORS', CORS::class)) { // allow credentials headers must not be true or CSRF is possible // otherwise foreach ($response->getHeaders() as $header => $value) { @@ -151,15 +97,9 @@ class CORSMiddleware extends Middleware { /** * If an SecurityException is being caught return a JSON error response - * - * @param Controller $controller the controller that is being called - * @param string $methodName the name of the method that will be called on - * the controller - * @param \Exception $exception the thrown exception - * @throws \Exception the passed in exception if it can't handle it - * @return Response a Response object or null in case that the exception could not be handled */ - public function afterException($controller, $methodName, \Exception $exception) { + #[Override] + public function afterException(Controller $controller, string $methodName, \Exception $exception): Response { if ($exception instanceof SecurityException) { $response = new JSONResponse(['message' => $exception->getMessage()]); if ($exception->getCode() !== 0) { diff --git a/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php b/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php index 3bc733931d4..7334b56bac1 100644 --- a/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php @@ -9,20 +9,18 @@ declare(strict_types=1); namespace OC\AppFramework\Middleware\Security; use OC\AppFramework\Http\Request; +use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\Security\Exceptions\LaxSameSiteCookieFailedException; -use OC\AppFramework\Utility\ControllerMethodReflector; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoSameSiteCookieRequired; use OCP\AppFramework\Http\Response; use OCP\AppFramework\Middleware; -use Psr\Log\LoggerInterface; use ReflectionMethod; class SameSiteCookieMiddleware extends Middleware { public function __construct( private readonly Request $request, - private readonly ControllerMethodReflector $reflector, - private readonly LoggerInterface $logger, + private readonly MiddlewareUtils $middlewareUtils, ) { } @@ -36,7 +34,7 @@ class SameSiteCookieMiddleware extends Middleware { } $reflectionMethod = new ReflectionMethod($controller, $methodName); - $noSSC = $this->hasAnnotationOrAttribute($reflectionMethod, 'NoSameSiteCookieRequired', NoSameSiteCookieRequired::class); + $noSSC = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoSameSiteCookieRequired', NoSameSiteCookieRequired::class); if ($noSSC) { return; } @@ -87,25 +85,4 @@ class SameSiteCookieMiddleware extends Middleware { ); } } - - /** - * @template T - * - * @param ReflectionMethod $reflectionMethod - * @param ?string $annotationName - * @param class-string $attributeClass - * @return boolean - */ - protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, ?string $annotationName, string $attributeClass): bool { - if (!empty($reflectionMethod->getAttributes($attributeClass))) { - return true; - } - - if ($annotationName && $this->reflector->hasAnnotation($annotationName)) { - $this->logger->debug($reflectionMethod->getDeclaringClass()->getName() . '::' . $reflectionMethod->getName() . ' uses the @' . $annotationName . ' annotation and should use the #[' . $attributeClass . '] attribute instead'); - return true; - } - - return false; - } } diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index e3a293e0fd9..d0ca1b2ebe1 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -8,6 +8,7 @@ declare(strict_types=1); */ namespace OC\AppFramework\Middleware\Security; +use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\Security\Exceptions\AdminIpNotAllowedException; use OC\AppFramework\Middleware\Security\Exceptions\AppNotEnabledException; use OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException; @@ -16,7 +17,6 @@ use OC\AppFramework\Middleware\Security\Exceptions\NotAdminException; use OC\AppFramework\Middleware\Security\Exceptions\NotLoggedInException; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Middleware\Security\Exceptions\StrictCookieMissingException; -use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Settings\AuthorizedGroupMapper; use OC\User\Session; use OCP\App\AppPathNotFoundException; @@ -59,20 +59,20 @@ class SecurityMiddleware extends Middleware { private ?bool $isSubAdmin = null; public function __construct( - private IRequest $request, - private ControllerMethodReflector $reflector, - private INavigationManager $navigationManager, - private IURLGenerator $urlGenerator, - private LoggerInterface $logger, - private string $appName, - private bool $isLoggedIn, - private IGroupManager $groupManager, - private ISubAdmin $subAdminManager, - private IAppManager $appManager, - private IL10N $l10n, - private AuthorizedGroupMapper $groupAuthorizationMapper, - private IUserSession $userSession, - private IRemoteAddress $remoteAddress, + private readonly IRequest $request, + private readonly MiddlewareUtils $middlewareUtils, + private readonly INavigationManager $navigationManager, + private readonly IURLGenerator $urlGenerator, + private readonly LoggerInterface $logger, + private readonly string $appName, + private readonly bool $isLoggedIn, + private readonly IGroupManager $groupManager, + private readonly ISubAdmin $subAdminManager, + private readonly IAppManager $appManager, + private readonly IL10N $l10n, + private readonly AuthorizedGroupMapper $groupAuthorizationMapper, + private readonly IUserSession $userSession, + private readonly IRemoteAddress $remoteAddress, ) { } @@ -115,15 +115,15 @@ class SecurityMiddleware extends Middleware { $reflectionMethod = new ReflectionMethod($controller, $methodName); // security checks - $isPublicPage = $this->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class); + $isPublicPage = $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'PublicPage', PublicPage::class); - if ($this->hasAnnotationOrAttribute($reflectionMethod, 'ExAppRequired', ExAppRequired::class)) { + if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'ExAppRequired', ExAppRequired::class)) { if (!$this->userSession instanceof Session || $this->userSession->getSession()->get('app_api') !== true) { throw new ExAppRequiredException(); } } elseif (!$isPublicPage) { $authorized = false; - if ($this->hasAnnotationOrAttribute($reflectionMethod, null, AppApiAdminAccessWithoutUser::class)) { + if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, null, AppApiAdminAccessWithoutUser::class)) { // this attribute allows ExApp to access admin endpoints only if "userId" is "null" if ($this->userSession instanceof Session && $this->userSession->getSession()->get('app_api') === true && $this->userSession->getUser() === null) { $authorized = true; @@ -134,15 +134,15 @@ class SecurityMiddleware extends Middleware { throw new NotLoggedInException(); } - if (!$authorized && $this->hasAnnotationOrAttribute($reflectionMethod, 'AuthorizedAdminSetting', AuthorizedAdminSetting::class)) { + if (!$authorized && $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'AuthorizedAdminSetting', AuthorizedAdminSetting::class)) { $authorized = $this->isAdminUser(); - if (!$authorized && $this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)) { + if (!$authorized && $this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)) { $authorized = $this->isSubAdmin(); } if (!$authorized) { - $settingClasses = $this->getAuthorizedAdminSettingClasses($reflectionMethod); + $settingClasses = $this->middlewareUtils->getAuthorizedAdminSettingClasses($reflectionMethod); $authorizedClasses = $this->groupAuthorizationMapper->findAllClassesForUser($this->userSession->getUser()); foreach ($settingClasses as $settingClass) { $authorized = in_array($settingClass, $authorizedClasses, true); @@ -159,24 +159,24 @@ class SecurityMiddleware extends Middleware { throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions')); } } - if ($this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) + if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) && !$this->isSubAdmin() && !$this->isAdminUser() && !$authorized) { throw new NotAdminException($this->l10n->t('Logged in account must be an admin or sub admin')); } - if (!$this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) - && !$this->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class) + if (!$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) + && !$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class) && !$this->isAdminUser() && !$authorized) { throw new NotAdminException($this->l10n->t('Logged in account must be an admin')); } - if ($this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) + if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) && !$this->remoteAddress->allowsAdminActions()) { throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions')); } - if (!$this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) - && !$this->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class) + if (!$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) + && !$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class) && !$this->remoteAddress->allowsAdminActions()) { throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn\'t allow you to perform admin actions')); } @@ -184,8 +184,8 @@ class SecurityMiddleware extends Middleware { } // Check for strict cookie requirement - if ($this->hasAnnotationOrAttribute($reflectionMethod, 'StrictCookieRequired', StrictCookiesRequired::class) - || !$this->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) { + if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'StrictCookieRequired', StrictCookiesRequired::class) + || !$this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) { if (!$this->request->passesStrictCookieCheck()) { throw new StrictCookieMissingException(); } @@ -224,7 +224,7 @@ class SecurityMiddleware extends Middleware { } private function isInvalidCSRFRequired(ReflectionMethod $reflectionMethod): bool { - if ($this->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) { + if ($this->middlewareUtils->hasAnnotationOrAttribute($reflectionMethod, 'NoCSRFRequired', NoCSRFRequired::class)) { return false; } @@ -236,49 +236,6 @@ class SecurityMiddleware extends Middleware { || str_starts_with($this->request->getHeader('Authorization'), 'Bearer '); } - /** - * @template T - * - * @param ReflectionMethod $reflectionMethod - * @param ?string $annotationName - * @param class-string $attributeClass - * @return boolean - */ - protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, ?string $annotationName, string $attributeClass): bool { - if (!empty($reflectionMethod->getAttributes($attributeClass))) { - return true; - } - - if ($annotationName && $this->reflector->hasAnnotation($annotationName)) { - $this->logger->debug($reflectionMethod->getDeclaringClass()->getName() . '::' . $reflectionMethod->getName() . ' uses the @' . $annotationName . ' annotation and should use the #[' . $attributeClass . '] attribute instead'); - return true; - } - - return false; - } - - /** - * @param ReflectionMethod $reflectionMethod - * @return string[] - */ - protected function getAuthorizedAdminSettingClasses(ReflectionMethod $reflectionMethod): array { - $classes = []; - if ($this->reflector->hasAnnotation('AuthorizedAdminSetting')) { - $classes = explode(';', $this->reflector->getAnnotationParameter('AuthorizedAdminSetting', 'settings')); - } - - $attributes = $reflectionMethod->getAttributes(AuthorizedAdminSetting::class); - if (!empty($attributes)) { - foreach ($attributes as $attribute) { - /** @var AuthorizedAdminSetting $setting */ - $setting = $attribute->newInstance(); - $classes[] = $setting->getSettings(); - } - } - - return $classes; - } - /** * If an SecurityException is being caught, ajax requests return a JSON error * response and non ajax requests redirect to the index diff --git a/tests/Core/Middleware/TwoFactorMiddlewareTest.php b/tests/Core/Middleware/TwoFactorMiddlewareTest.php index bf8899bd52a..062b1d94b5e 100644 --- a/tests/Core/Middleware/TwoFactorMiddlewareTest.php +++ b/tests/Core/Middleware/TwoFactorMiddlewareTest.php @@ -10,6 +10,8 @@ namespace Test\Core\Middleware; use OC\AppFramework\Http\Attributes\TwoFactorSetUpDoneRequired; use OC\AppFramework\Http\Request; +use OC\AppFramework\Middleware\MiddlewareUtils; +use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Authentication\Exceptions\TwoFactorAuthRequiredException; use OC\Authentication\Exceptions\UserAlreadyLoggedInException; use OC\Authentication\TwoFactorAuth\Manager; @@ -21,7 +23,6 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\NoTwoFactorRequired; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\Response; -use OCP\AppFramework\Utility\IControllerMethodReflector; use OCP\Authentication\TwoFactorAuth\ALoginSetupController; use OCP\Authentication\TwoFactorAuth\IProvider; use OCP\IConfig; @@ -73,7 +74,7 @@ class TwoFactorMiddlewareTest extends TestCase { private IUserSession&MockObject $userSession; private ISession&MockObject $session; private IURLGenerator&MockObject $urlGenerator; - private IControllerMethodReflector&MockObject $reflector; + private ControllerMethodReflector&MockObject $reflector; private IRequest $request; private TwoFactorMiddleware $middleware; private LoggerInterface&MockObject $logger; @@ -89,7 +90,7 @@ class TwoFactorMiddlewareTest extends TestCase { ->getMock(); $this->session = $this->createMock(ISession::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); - $this->reflector = $this->createMock(IControllerMethodReflector::class); + $this->reflector = $this->createMock(ControllerMethodReflector::class); $this->logger = $this->createMock(LoggerInterface::class); $this->request = new Request( [ @@ -101,7 +102,7 @@ class TwoFactorMiddlewareTest extends TestCase { $this->createMock(IConfig::class) ); - $this->middleware = new TwoFactorMiddleware($this->twoFactorManager, $this->userSession, $this->session, $this->urlGenerator, $this->reflector, $this->request, $this->logger); + $this->middleware = new TwoFactorMiddleware($this->twoFactorManager, $this->userSession, $this->session, $this->urlGenerator, new MiddlewareUtils($this->reflector, $this->logger), $this->request); } public function testBeforeControllerNotLoggedIn(): void { diff --git a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php index c325ae638fb..97541612429 100644 --- a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php @@ -8,6 +8,7 @@ namespace Test\AppFramework\Middleware\Security; use OC\AppFramework\Http\Request; +use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\Security\CORSMiddleware; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Utility\ControllerMethodReflector; @@ -24,14 +25,10 @@ use Psr\Log\LoggerInterface; use Test\AppFramework\Middleware\Security\Mock\CORSMiddlewareController; class CORSMiddlewareTest extends \Test\TestCase { - /** @var ControllerMethodReflector */ - private $reflector; - /** @var Session|MockObject */ - private $session; - /** @var IThrottler|MockObject */ - private $throttler; - /** @var CORSMiddlewareController */ - private $controller; + private ControllerMethodReflector $reflector; + private Session&MockObject $session; + private IThrottler&MockObject $throttler; + private CORSMiddlewareController $controller; private LoggerInterface $logger; protected function setUp(): void { @@ -65,7 +62,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); $response = $middleware->afterController($this->controller, $method, new Response()); $headers = $response->getHeaders(); @@ -82,7 +79,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->createMock(IRequestId::class), $this->createMock(IConfig::class) ); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); $response = $middleware->afterController($this->controller, __FUNCTION__, new Response()); $headers = $response->getHeaders(); @@ -104,7 +101,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); $response = $middleware->afterController($this->controller, $method, new Response()); $headers = $response->getHeaders(); @@ -132,7 +129,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler, $this->logger); $response = new Response(); $response->addHeader('AcCess-control-Allow-Credentials ', 'TRUE'); @@ -156,7 +153,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler, $this->logger); $this->session->expects($this->once()) ->method('isLoggedIn') ->willReturn(false); @@ -188,7 +185,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); $this->session->expects($this->once()) ->method('isLoggedIn') ->willReturn(true); @@ -227,7 +224,7 @@ class CORSMiddlewareTest extends \Test\TestCase { ->with($this->equalTo('user'), $this->equalTo('pass')) ->willReturn(true); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); $middleware->beforeController($this->controller, $method); } @@ -258,7 +255,7 @@ class CORSMiddlewareTest extends \Test\TestCase { ->with($this->equalTo('user'), $this->equalTo('pass')) ->willThrowException(new PasswordLoginForbiddenException); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); $middleware->beforeController($this->controller, $method); } @@ -289,7 +286,7 @@ class CORSMiddlewareTest extends \Test\TestCase { ->with($this->equalTo('user'), $this->equalTo('pass')) ->willReturn(false); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); $middleware->beforeController($this->controller, $method); } @@ -303,7 +300,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->createMock(IRequestId::class), $this->createMock(IConfig::class) ); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); $response = $middleware->afterException($this->controller, __FUNCTION__, new SecurityException('A security exception')); $expected = new JSONResponse(['message' => 'A security exception'], 500); @@ -319,7 +316,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->createMock(IRequestId::class), $this->createMock(IConfig::class) ); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); $response = $middleware->afterException($this->controller, __FUNCTION__, new SecurityException('A security exception', 501)); $expected = new JSONResponse(['message' => 'A security exception'], 501); @@ -338,7 +335,7 @@ class CORSMiddlewareTest extends \Test\TestCase { $this->createMock(IRequestId::class), $this->createMock(IConfig::class) ); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); + $middleware = new CORSMiddleware($request, new MiddlewareUtils($this->reflector, $this->logger), $this->session, $this->throttler); $middleware->afterException($this->controller, __FUNCTION__, new \Exception('A regular exception')); } } diff --git a/tests/lib/AppFramework/Middleware/Security/SameSiteCookieMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SameSiteCookieMiddlewareTest.php index 54d31ce37f3..ebcd3d4613b 100644 --- a/tests/lib/AppFramework/Middleware/Security/SameSiteCookieMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SameSiteCookieMiddlewareTest.php @@ -8,6 +8,7 @@ namespace Test\AppFramework\Middleware\Security; use OC\AppFramework\Http\Request; +use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\Security\Exceptions\LaxSameSiteCookieFailedException; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Middleware\Security\SameSiteCookieMiddleware; @@ -43,9 +44,9 @@ class SameSiteCookieMiddlewareTest extends TestCase { parent::setUp(); $this->request = $this->createMock(Request::class); - $this->reflector = $this->createMock(ControllerMethodReflector::class); $this->logger = $this->createMock(LoggerInterface::class); - $this->middleware = new SameSiteCookieMiddleware($this->request, $this->reflector, $this->logger); + $this->reflector = $this->createMock(ControllerMethodReflector::class); + $this->middleware = new SameSiteCookieMiddleware($this->request, new MiddlewareUtils($this->reflector, $this->logger)); } public function testBeforeControllerNoIndex(): void { @@ -117,7 +118,7 @@ class SameSiteCookieMiddlewareTest extends TestCase { ->willReturn('/myrequri'); $middleware = $this->getMockBuilder(SameSiteCookieMiddleware::class) - ->setConstructorArgs([$this->request, $this->reflector, $this->logger]) + ->setConstructorArgs([$this->request, new MiddlewareUtils($this->reflector, $this->logger)]) ->onlyMethods(['setSameSiteCookie']) ->getMock(); diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index 0c6fc21357d..62886edd7b2 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -10,6 +10,7 @@ namespace Test\AppFramework\Middleware\Security; use OC\AppFramework\Http; use OC\AppFramework\Http\Request; +use OC\AppFramework\Middleware\MiddlewareUtils; use OC\AppFramework\Middleware\Security\Exceptions\AppNotEnabledException; use OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException; use OC\AppFramework\Middleware\Security\Exceptions\ExAppRequiredException; @@ -37,38 +38,26 @@ use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserSession; use OCP\Security\Ip\IRemoteAddress; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\AppFramework\Middleware\Security\Mock\NormalController; use Test\AppFramework\Middleware\Security\Mock\OCSController; use Test\AppFramework\Middleware\Security\Mock\SecurityMiddlewareController; class SecurityMiddlewareTest extends \Test\TestCase { - /** @var SecurityMiddleware|\PHPUnit\Framework\MockObject\MockObject */ - private $middleware; - /** @var SecurityMiddlewareController */ - private $controller; - /** @var SecurityException */ - private $secException; - /** @var SecurityException */ - private $secAjaxException; - /** @var IRequest|\PHPUnit\Framework\MockObject\MockObject */ - private $request; - /** @var ControllerMethodReflector */ - private $reader; - /** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */ - private $logger; - /** @var INavigationManager|\PHPUnit\Framework\MockObject\MockObject */ - private $navigationManager; - /** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */ - private $urlGenerator; - /** @var IAppManager|\PHPUnit\Framework\MockObject\MockObject */ - private $appManager; - /** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */ - private $l10n; - /** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */ - private $userSession; - /** @var AuthorizedGroupMapper|\PHPUnit\Framework\MockObject\MockObject */ - private $authorizedGroupMapper; + private SecurityMiddleware $middleware; + private ControllerMethodReflector $reader; + private SecurityMiddlewareController $controller; + private SecurityException $secAjaxException; + private IRequest|MockObject $request; + private MiddlewareUtils $middlewareUtils; + private LoggerInterface&MockObject $logger; + private INavigationManager&MockObject $navigationManager; + private IURLGenerator&MockObject $urlGenerator; + private IAppManager&MockObject $appManager; + private IL10N&MockObject $l10n; + private IUserSession&MockObject $userSession; + private AuthorizedGroupMapper&MockObject $authorizedGroupMapper; protected function setUp(): void { parent::setUp(); @@ -88,8 +77,8 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->navigationManager = $this->createMock(INavigationManager::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->l10n = $this->createMock(IL10N::class); + $this->middlewareUtils = new MiddlewareUtils($this->reader, $this->logger); $this->middleware = $this->getMiddleware(true, true, false); - $this->secException = new SecurityException('hey', false); $this->secAjaxException = new SecurityException('hey', true); } @@ -110,7 +99,7 @@ class SecurityMiddlewareTest extends \Test\TestCase { return new SecurityMiddleware( $this->request, - $this->reader, + $this->middlewareUtils, $this->navigationManager, $this->urlGenerator, $this->logger, From 8bb13df6cf1dc4147802175ed3395f0d73f70362 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Wed, 28 Jan 2026 21:47:38 +0100 Subject: [PATCH 4/4] refactor(AppFramework): Change version to 34 This didn't manage to get into NC 33 Signed-off-by: Carl Schwan --- .../AppFramework/Http/Attribute/NoSameSiteCookieRequired.php | 4 ++-- .../AppFramework/Http/Attribute/NoTwoFactorRequired.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php b/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php index 5bbb65cbe29..5bfa3b54242 100644 --- a/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php +++ b/lib/public/AppFramework/Http/Attribute/NoSameSiteCookieRequired.php @@ -17,9 +17,9 @@ use OCP\AppFramework\Attribute\Consumable; * Attribute for controller methods that want to disable the same site cookies * requirements. * - * @since 33.0.0 + * @since 34.0.0 */ #[Attribute(Attribute::TARGET_METHOD)] -#[Consumable(since: '33.0.0')] +#[Consumable(since: '34.0.0')] class NoSameSiteCookieRequired { } diff --git a/lib/public/AppFramework/Http/Attribute/NoTwoFactorRequired.php b/lib/public/AppFramework/Http/Attribute/NoTwoFactorRequired.php index c70a41a3bb1..ad14f726185 100644 --- a/lib/public/AppFramework/Http/Attribute/NoTwoFactorRequired.php +++ b/lib/public/AppFramework/Http/Attribute/NoTwoFactorRequired.php @@ -21,10 +21,10 @@ use OCP\AppFramework\Attribute\Consumable; * (use this wisely and only in two-factor auth apps, e.g. to allow setup during * login). * - * @since 33.0.0 + * @since 34.0.0 */ #[Attribute(Attribute::TARGET_METHOD)] -#[Consumable(since: '33.0.0')] +#[Consumable(since: '34.0.0')] class NoTwoFactorRequired { }