From e5dcdfb9e012dbe2811832e4bc0c233bdb3fcf21 Mon Sep 17 00:00:00 2001 From: provokateurin Date: Mon, 15 Jul 2024 15:25:45 +0200 Subject: [PATCH] feat(Security): Warn about using annotations instead of attributes Signed-off-by: provokateurin --- .../DependencyInjection/DIContainer.php | 4 ++- .../Middleware/Security/CORSMiddleware.php | 6 ++++- .../PasswordConfirmationMiddleware.php | 3 +++ .../Security/SecurityMiddleware.php | 1 + .../Security/CORSMiddlewareTest.php | 27 ++++++++++--------- .../PasswordConfirmationMiddlewareTest.php | 4 +++ 6 files changed, 31 insertions(+), 14 deletions(-) diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 4add17396b0d0..c25b6994b4fd6 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -207,7 +207,8 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta $c->get(IRequest::class), $c->get(IControllerMethodReflector::class), $c->get(IUserSession::class), - $c->get(IThrottler::class) + $c->get(IThrottler::class), + $c->get(LoggerInterface::class) ) ); $dispatcher->registerMiddleware( @@ -251,6 +252,7 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta $c->get(IUserSession::class), $c->get(ITimeFactory::class), $c->get(\OC\Authentication\Token\IProvider::class), + $c->get(LoggerInterface::class), ) ); $dispatcher->registerMiddleware( diff --git a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php index 7b617b22e3c25..3f0755b1b91e3 100644 --- a/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php @@ -21,6 +21,7 @@ use OCP\IRequest; use OCP\ISession; use OCP\Security\Bruteforce\IThrottler; +use Psr\Log\LoggerInterface; use ReflectionMethod; /** @@ -42,7 +43,9 @@ class CORSMiddleware extends Middleware { public function __construct(IRequest $request, ControllerMethodReflector $reflector, Session $session, - IThrottler $throttler) { + IThrottler $throttler, + private readonly LoggerInterface $logger, + ) { $this->request = $request; $this->reflector = $reflector; $this->session = $session; @@ -103,6 +106,7 @@ protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, if (!empty($reflectionMethod->getAttributes($attributeClass))) { + $this->logger->debug($reflectionMethod->getDeclaringClass()->getName() . '::' . $reflectionMethod->getName() . ' uses the @' . $annotationName . ' annotation and should use the #[' . $attributeClass . '] attribute instead'); return true; } diff --git a/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php b/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php index 5ff9d7386da01..a983de23597e8 100644 --- a/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php @@ -20,6 +20,7 @@ use OCP\IUserSession; use OCP\Session\Exceptions\SessionNotAvailableException; use OCP\User\Backend\IPasswordConfirmationBackend; +use Psr\Log\LoggerInterface; use ReflectionMethod; class PasswordConfirmationMiddleware extends Middleware { @@ -48,6 +49,7 @@ public function __construct(ControllerMethodReflector $reflector, IUserSession $userSession, ITimeFactory $timeFactory, IProvider $tokenProvider, + private readonly LoggerInterface $logger, ) { $this->reflector = $reflector; $this->session = $session; @@ -113,6 +115,7 @@ protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, } 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; } diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index bc2014da246c6..603b5d543dc2d 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -243,6 +243,7 @@ protected function hasAnnotationOrAttribute(ReflectionMethod $reflectionMethod, } 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; } diff --git a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php index ab06b020c9bdf..00538dda4b08a 100644 --- a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php @@ -18,6 +18,7 @@ use OCP\IRequestId; use OCP\Security\Bruteforce\IThrottler; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; use Test\AppFramework\Middleware\Security\Mock\CORSMiddlewareController; class CORSMiddlewareTest extends \Test\TestCase { @@ -29,12 +30,14 @@ class CORSMiddlewareTest extends \Test\TestCase { private $throttler; /** @var CORSMiddlewareController */ private $controller; + private LoggerInterface $logger; protected function setUp(): void { parent::setUp(); $this->reflector = new ControllerMethodReflector(); $this->session = $this->createMock(Session::class); $this->throttler = $this->createMock(IThrottler::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->controller = new CORSMiddlewareController( 'test', $this->createMock(IRequest::class) @@ -62,7 +65,7 @@ public function testSetCORSAPIHeader(string $method): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $response = $middleware->afterController($this->controller, $method, new Response()); $headers = $response->getHeaders(); @@ -79,7 +82,7 @@ public function testNoAnnotationNoCORSHEADER(): void { $this->createMock(IRequestId::class), $this->createMock(IConfig::class) ); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $response = $middleware->afterController($this->controller, __FUNCTION__, new Response()); $headers = $response->getHeaders(); @@ -103,7 +106,7 @@ public function testNoOriginHeaderNoCORSHEADER(string $method): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $response = $middleware->afterController($this->controller, $method, new Response()); $headers = $response->getHeaders(); @@ -133,7 +136,7 @@ public function testCorsIgnoredIfWithCredentialsHeaderPresent(string $method): v $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $response = new Response(); $response->addHeader('AcCess-control-Allow-Credentials ', 'TRUE'); @@ -159,7 +162,7 @@ public function testNoCORSOnAnonymousPublicPage(string $method): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $this->session->expects($this->once()) ->method('isLoggedIn') ->willReturn(false); @@ -193,7 +196,7 @@ public function testCORSShouldNeverAllowCookieAuth(string $method): void { $this->createMock(IConfig::class) ); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $this->session->expects($this->once()) ->method('isLoggedIn') ->willReturn(true); @@ -234,7 +237,7 @@ public function testCORSShouldRelogin(string $method): void { ->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); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $middleware->beforeController($this->controller, $method); } @@ -267,7 +270,7 @@ public function testCORSShouldFailIfPasswordLoginIsForbidden(string $method): vo ->with($this->equalTo('user'), $this->equalTo('pass')) ->will($this->throwException(new \OC\Authentication\Exceptions\PasswordLoginForbiddenException)); $this->reflector->reflect($this->controller, $method); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $middleware->beforeController($this->controller, $method); } @@ -300,7 +303,7 @@ public function testCORSShouldNotAllowCookieAuth(string $method): void { ->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); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $middleware->beforeController($this->controller, $method); } @@ -314,7 +317,7 @@ public function testAfterExceptionWithSecurityExceptionNoStatus() { $this->createMock(IRequestId::class), $this->createMock(IConfig::class) ); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $response = $middleware->afterException($this->controller, __FUNCTION__, new SecurityException('A security exception')); $expected = new JSONResponse(['message' => 'A security exception'], 500); @@ -330,7 +333,7 @@ public function testAfterExceptionWithSecurityExceptionWithStatus() { $this->createMock(IRequestId::class), $this->createMock(IConfig::class) ); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $response = $middleware->afterException($this->controller, __FUNCTION__, new SecurityException('A security exception', 501)); $expected = new JSONResponse(['message' => 'A security exception'], 501); @@ -349,7 +352,7 @@ public function testAfterExceptionWithRegularException() { $this->createMock(IRequestId::class), $this->createMock(IConfig::class) ); - $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler); + $middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger); $middleware->afterException($this->controller, __FUNCTION__, new \Exception('A regular exception')); } } diff --git a/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php index beee7151264d3..a9f3258e8d306 100644 --- a/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php @@ -16,6 +16,7 @@ use OCP\ISession; use OCP\IUser; use OCP\IUserSession; +use Psr\Log\LoggerInterface; use Test\AppFramework\Middleware\Security\Mock\PasswordConfirmationMiddlewareController; use Test\TestCase; @@ -35,6 +36,7 @@ class PasswordConfirmationMiddlewareTest extends TestCase { /** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */ private $timeFactory; private IProvider|\PHPUnit\Framework\MockObject\MockObject $tokenProvider; + private LoggerInterface $logger; protected function setUp(): void { $this->reflector = new ControllerMethodReflector(); @@ -43,6 +45,7 @@ protected function setUp(): void { $this->user = $this->createMock(IUser::class); $this->timeFactory = $this->createMock(ITimeFactory::class); $this->tokenProvider = $this->createMock(IProvider::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->controller = new PasswordConfirmationMiddlewareController( 'test', $this->createMock(IRequest::class) @@ -54,6 +57,7 @@ protected function setUp(): void { $this->userSession, $this->timeFactory, $this->tokenProvider, + $this->logger, ); }