feat: move csrf validation out of requestdept-remove-csrf-dependency-from-request

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>

2 files changed, 78 insertions, 25 deletions

diff --git a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php
index 00538dda4b0..1e01f99a5d9 100644
--- a/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php
+++ b/tests/lib/AppFramework/Middleware/Security/CORSMiddlewareTest.php
@@ -10,6 +10,8 @@ use OC\AppFramework\Http\Request;
 use OC\AppFramework\Middleware\Security\CORSMiddleware;
 use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
 use OC\AppFramework\Utility\ControllerMethodReflector;
+use OC\Security\CSRF\CsrfTokenManager;
+use OC\Security\CSRF\CsrfValidator;
 use OC\User\Session;
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\AppFramework\Http\Response;
@@ -28,6 +30,8 @@ class CORSMiddlewareTest extends \Test\TestCase {
 	private $session;
 	/** @var IThrottler|MockObject */
 	private $throttler;
+	private CsrfTokenManager $csrfTokenManager;
+	private CsrfValidator $csrfValidator;
 	/** @var CORSMiddlewareController */
 	private $controller;
 	private LoggerInterface $logger;
@@ -38,6 +42,8 @@ class CORSMiddlewareTest extends \Test\TestCase {
 		$this->session = $this->createMock(Session::class);
 		$this->throttler = $this->createMock(IThrottler::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->csrfTokenManager = $this->createMock(CsrfTokenManager::class);
+		$this->csrfValidator = new CsrfValidator($this->csrfTokenManager);
 		$this->controller = new CORSMiddlewareController(
 			'test',
 			$this->createMock(IRequest::class)
@@ -65,7 +71,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, $this->reflector, $this->session, $this->throttler, $this->logger, $this->csrfValidator);
 
 		$response = $middleware->afterController($this->controller, $method, new Response());
 		$headers = $response->getHeaders();
@@ -82,7 +88,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, $this->reflector, $this->session, $this->throttler, $this->logger, $this->csrfValidator);
 
 		$response = $middleware->afterController($this->controller, __FUNCTION__, new Response());
 		$headers = $response->getHeaders();
@@ -106,7 +112,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, $this->reflector, $this->session, $this->throttler, $this->logger, $this->csrfValidator);
 
 		$response = $middleware->afterController($this->controller, $method, new Response());
 		$headers = $response->getHeaders();
@@ -136,7 +142,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, $this->reflector, $this->session, $this->throttler, $this->logger, $this->csrfValidator);
 
 		$response = new Response();
 		$response->addHeader('AcCess-control-Allow-Credentials ', 'TRUE');
@@ -162,7 +168,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, $this->reflector, $this->session, $this->throttler, $this->logger, $this->csrfValidator);
 		$this->session->expects($this->once())
 			->method('isLoggedIn')
 			->willReturn(false);
@@ -196,7 +202,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, $this->reflector, $this->session, $this->throttler, $this->logger, $this->csrfValidator);
 		$this->session->expects($this->once())
 			->method('isLoggedIn')
 			->willReturn(true);
@@ -237,7 +243,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, $this->reflector, $this->session, $this->throttler, $this->logger, $this->csrfValidator);
 
 		$middleware->beforeController($this->controller, $method);
 	}
@@ -270,7 +276,7 @@ class CORSMiddlewareTest extends \Test\TestCase {
 			->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, $this->logger);
+		$middleware = new CORSMiddleware($request, $this->reflector, $this->session, $this->throttler, $this->logger, $this->csrfValidator);
 
 		$middleware->beforeController($this->controller, $method);
 	}
@@ -303,7 +309,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, $this->reflector, $this->session, $this->throttler, $this->logger, $this->csrfValidator);
 
 		$middleware->beforeController($this->controller, $method);
 	}
@@ -317,7 +323,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, $this->reflector, $this->session, $this->throttler, $this->logger, $this->csrfValidator);
 		$response = $middleware->afterException($this->controller, __FUNCTION__, new SecurityException('A security exception'));
 
 		$expected = new JSONResponse(['message' => 'A security exception'], 500);
@@ -333,7 +339,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, $this->reflector, $this->session, $this->throttler, $this->logger, $this->csrfValidator);
 		$response = $middleware->afterException($this->controller, __FUNCTION__, new SecurityException('A security exception', 501));
 
 		$expected = new JSONResponse(['message' => 'A security exception'], 501);
@@ -352,7 +358,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, $this->reflector, $this->session, $this->throttler, $this->logger, $this->csrfValidator);
 		$middleware->afterException($this->controller, __FUNCTION__, new \Exception('A regular exception'));
 	}
 }
diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
index fb457579fac..f4a2bd7e884 100644
--- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
+++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
@@ -18,6 +18,9 @@ use OC\AppFramework\Middleware\Security\Exceptions\SecurityException;
 use OC\Appframework\Middleware\Security\Exceptions\StrictCookieMissingException;
 use OC\AppFramework\Middleware\Security\SecurityMiddleware;
 use OC\AppFramework\Utility\ControllerMethodReflector;
+use OC\Security\CSRF\CsrfToken;
+use OC\Security\CSRF\CsrfTokenManager;
+use OC\Security\CSRF\CsrfValidator;
 use OC\Settings\AuthorizedGroupMapper;
 use OC\User\Session;
 use OCP\App\IAppManager;
@@ -65,6 +68,8 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 	private $userSession;
 	/** @var AuthorizedGroupMapper|\PHPUnit\Framework\MockObject\MockObject */
 	private $authorizedGroupMapper;
+	private CsrfTokenManager $csrfTokenManager;
+	private CsrfValidator $csrfValidator;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -81,6 +86,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->csrfTokenManager = $this->createMock(CsrfTokenManager::class);
+		$this->csrfValidator = new CsrfValidator($this->csrfTokenManager);
 		$this->middleware = $this->getMiddleware(true, true, false);
 		$this->secException = new SecurityException('hey', false);
 		$this->secAjaxException = new SecurityException('hey', true);
@@ -108,7 +115,8 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 			$this->l10n,
 			$this->authorizedGroupMapper,
 			$this->userSession,
-			$remoteIpAddress
+			$remoteIpAddress,
+			$this->csrfValidator,
 		);
 	}
 
@@ -321,12 +329,18 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 	public function testCsrfCheck(string $method): void {
 		$this->expectException(\OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException::class);
 
-		$this->request->expects($this->once())
-			->method('passesCSRFCheck')
-			->willReturn(false);
-		$this->request->expects($this->once())
+		$this->request->expects($this->exactly(2))
 			->method('passesStrictCookieCheck')
 			->willReturn(true);
+		$this->request->expects($this->once())
+			->method('getParam')
+			->with('requesttoken', '')
+			->willReturn('');
+		$this->request->expects($this->once())
+			->method('getHeader')
+			->with('REQUESTTOKEN')
+			->willReturn('');
+
 		$this->reader->reflect($this->controller, $method);
 		$this->middleware->beforeController($this->controller, $method);
 	}
@@ -347,11 +361,20 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 	 * @dataProvider dataPublicPage
 	 */
 	public function testPassesCsrfCheck(string $method): void {
-		$this->request->expects($this->once())
-			->method('passesCSRFCheck')
+		$this->request->expects($this->exactly(2))
+			->method('passesStrictCookieCheck')
 			->willReturn(true);
 		$this->request->expects($this->once())
-			->method('passesStrictCookieCheck')
+			->method('getParam')
+			->with('requesttoken', '')
+			->willReturn('');
+		$this->request->expects($this->once())
+			->method('getHeader')
+			->with('REQUESTTOKEN')
+			->willReturn('foobar');
+		$this->csrfTokenManager->expects($this->once())
+			->method('isTokenValid')
+			->with(new CsrfToken('foobar'))
 			->willReturn(true);
 
 		$this->reader->reflect($this->controller, $method);
@@ -364,12 +387,21 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 	public function testFailCsrfCheck(string $method): void {
 		$this->expectException(\OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException::class);
 
-		$this->request->expects($this->once())
-			->method('passesCSRFCheck')
-			->willReturn(false);
-		$this->request->expects($this->once())
+		$this->request->expects($this->exactly(2))
 			->method('passesStrictCookieCheck')
 			->willReturn(true);
+		$this->request->expects($this->once())
+			->method('getParam')
+			->with('requesttoken', '')
+			->willReturn('');
+		$this->request->expects($this->once())
+			->method('getHeader')
+			->with('REQUESTTOKEN')
+			->willReturn('foobar');
+		$this->csrfTokenManager->expects($this->once())
+			->method('isTokenValid')
+			->with(new CsrfToken('foobar'))
+			->willReturn(false);
 
 		$this->reader->reflect($this->controller, $method);
 		$this->middleware->beforeController($this->controller, $method);
@@ -386,6 +418,12 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 		$this->request->expects($this->once())
 			->method('passesStrictCookieCheck')
 			->willReturn(false);
+		$this->request->expects($this->never())
+			->method('getParam');
+		$this->request->expects($this->never())
+			->method('getHeader');
+		$this->csrfTokenManager->expects($this->never())
+			->method('isTokenValid');
 
 		$this->reader->reflect($this->controller, $method);
 		$this->middleware->beforeController($this->controller, $method);
@@ -441,6 +479,9 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 		$this->request
 			->method('getHeader')
 			->willReturnCallback(function ($header) use ($hasOcsApiHeader, $hasBearerAuth) {
+				if ($header === 'REQUESTTOKEN') {
+					return '';
+				}
 				if ($header === 'OCS-APIREQUEST' && $hasOcsApiHeader) {
 					return 'true';
 				}
@@ -449,9 +490,15 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 				}
 				return '';
 			});
-		$this->request->expects($this->once())
+		$this->request->expects($this->exactly(2))
 			->method('passesStrictCookieCheck')
 			->willReturn(true);
+		$this->request->expects($this->once())
+			->method('getParam')
+			->with('requesttoken', '')
+			->willReturn('');
+		$this->csrfTokenManager->expects($this->never())
+			->method('isTokenValid');
 
 		$controller = new $controllerClass('test', $this->request);