From 20e00cdf17f45f811135fe5fb61c133ce9021144 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Wed, 25 Jan 2023 19:26:21 +0100 Subject: [PATCH] feat(app-framework): Add UseSession attribute to replace annotation Signed-off-by: Christoph Wurst --- core/Controller/ClientFlowLoginController.php | 7 +- .../ClientFlowLoginV2Controller.php | 9 +- core/Controller/LoginController.php | 9 +- .../TwoFactorChallengeController.php | 5 +- core/Controller/WebAuthnController.php | 5 +- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../Middleware/SessionMiddleware.php | 34 +++- .../Http/Attribute/UseSession.php | 37 +++++ .../Middleware/SessionMiddlewareTest.php | 153 ++++++++++++------ 10 files changed, 189 insertions(+), 72 deletions(-) create mode 100644 lib/public/AppFramework/Http/Attribute/UseSession.php diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 4c262714fe62e..85a793bd92b1f 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -41,6 +41,7 @@ use OCA\OAuth2\Db\ClientMapper; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; +use OCP\AppFramework\Http\Attribute\UseSession; use OCP\AppFramework\Http\Response; use OCP\AppFramework\Http\StandaloneTemplateResponse; use OCP\Defaults; @@ -126,8 +127,8 @@ private function stateTokenForbiddenResponse(): StandaloneTemplateResponse { /** * @PublicPage * @NoCSRFRequired - * @UseSession */ + #[UseSession] public function showAuthPickerPage(string $clientIdentifier = '', string $user = '', int $direct = 0): StandaloneTemplateResponse { $clientName = $this->getClientName(); $client = null; @@ -193,8 +194,8 @@ public function showAuthPickerPage(string $clientIdentifier = '', string $user = * @NoAdminRequired * @NoCSRFRequired * @NoSameSiteCookieRequired - * @UseSession */ + #[UseSession] public function grantPage(string $stateToken = '', string $clientIdentifier = '', int $direct = 0): StandaloneTemplateResponse { @@ -243,10 +244,10 @@ public function grantPage(string $stateToken = '', /** * @NoAdminRequired - * @UseSession * * @return Http\RedirectResponse|Response */ + #[UseSession] public function generateAppPassword(string $stateToken, string $clientIdentifier = '') { if (!$this->isValidToken($stateToken)) { diff --git a/core/Controller/ClientFlowLoginV2Controller.php b/core/Controller/ClientFlowLoginV2Controller.php index 4df94a28d6a1e..d476b0cdc0323 100644 --- a/core/Controller/ClientFlowLoginV2Controller.php +++ b/core/Controller/ClientFlowLoginV2Controller.php @@ -33,6 +33,7 @@ use OC\Core\Service\LoginFlowV2Service; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; +use OCP\AppFramework\Http\Attribute\UseSession; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\Response; @@ -97,8 +98,8 @@ public function poll(string $token): JSONResponse { /** * @NoCSRFRequired * @PublicPage - * @UseSession */ + #[UseSession] public function landing(string $token, $user = ''): Response { if (!$this->loginFlowV2Service->startLoginFlow($token)) { return $this->loginTokenForbiddenResponse(); @@ -114,8 +115,8 @@ public function landing(string $token, $user = ''): Response { /** * @NoCSRFRequired * @PublicPage - * @UseSession */ + #[UseSession] public function showAuthPickerPage($user = ''): StandaloneTemplateResponse { try { $flow = $this->getFlowByLoginToken(); @@ -145,10 +146,10 @@ public function showAuthPickerPage($user = ''): StandaloneTemplateResponse { /** * @NoAdminRequired - * @UseSession * @NoCSRFRequired * @NoSameSiteCookieRequired */ + #[UseSession] public function grantPage(string $stateToken): StandaloneTemplateResponse { if (!$this->isValidStateToken($stateToken)) { return $this->stateTokenForbiddenResponse(); @@ -222,8 +223,8 @@ public function apptokenRedirect(string $stateToken, string $user, string $passw /** * @NoAdminRequired - * @UseSession */ + #[UseSession] public function generateAppPassword(string $stateToken): Response { if (!$this->isValidStateToken($stateToken)) { return $this->stateTokenForbiddenResponse(); diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index 8fd994ae6483b..b93c6002ed5f3 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -43,6 +43,7 @@ use OC_App; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; +use OCP\AppFramework\Http\Attribute\UseSession; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; @@ -105,10 +106,10 @@ public function __construct(?string $appName, /** * @NoAdminRequired - * @UseSession * * @return RedirectResponse */ + #[UseSession] public function logout() { $loginToken = $this->request->getCookie('nc_token'); if (!is_null($loginToken)) { @@ -134,13 +135,13 @@ public function logout() { /** * @PublicPage * @NoCSRFRequired - * @UseSession * * @param string $user * @param string $redirect_url * * @return TemplateResponse|RedirectResponse */ + #[UseSession] public function showLoginForm(string $user = null, string $redirect_url = null): Http\Response { if ($this->userSession->isLoggedIn()) { return new RedirectResponse($this->urlGenerator->linkToDefaultPageUrl()); @@ -283,12 +284,12 @@ private function generateRedirect(?string $redirectUrl): RedirectResponse { /** * @PublicPage - * @UseSession * @NoCSRFRequired * @BruteForceProtection(action=login) * * @return RedirectResponse */ + #[UseSession] public function tryLogin(Chain $loginChain, string $user, string $password, @@ -368,12 +369,12 @@ private function createLoginFailedResponse( /** * @NoAdminRequired - * @UseSession * @BruteForceProtection(action=sudo) * * @license GNU AGPL version 3 or any later version * */ + #[UseSession] public function confirmPassword(string $password): DataResponse { $loginName = $this->userSession->getLoginName(); $loginResult = $this->userManager->checkPassword($loginName, $password); diff --git a/core/Controller/TwoFactorChallengeController.php b/core/Controller/TwoFactorChallengeController.php index deebeb21d32e6..7a57d5eeb1a9c 100644 --- a/core/Controller/TwoFactorChallengeController.php +++ b/core/Controller/TwoFactorChallengeController.php @@ -28,6 +28,7 @@ use OC\Authentication\TwoFactorAuth\Manager; use OC_User; use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\Attribute\UseSession; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\StandaloneTemplateResponse; use OCP\Authentication\TwoFactorAuth\IActivatableAtLogin; @@ -110,13 +111,13 @@ public function selectChallenge($redirect_url) { /** * @NoAdminRequired * @NoCSRFRequired - * @UseSession * @TwoFactorSetUpDoneRequired * * @param string $challengeProviderId * @param string $redirect_url * @return StandaloneTemplateResponse|RedirectResponse */ + #[UseSession] public function showChallenge($challengeProviderId, $redirect_url) { $user = $this->userSession->getUser(); $providerSet = $this->twoFactorManager->getProviderSet($user); @@ -161,7 +162,6 @@ public function showChallenge($challengeProviderId, $redirect_url) { /** * @NoAdminRequired * @NoCSRFRequired - * @UseSession * @TwoFactorSetUpDoneRequired * * @UserRateThrottle(limit=5, period=100) @@ -171,6 +171,7 @@ public function showChallenge($challengeProviderId, $redirect_url) { * @param string $redirect_url * @return RedirectResponse */ + #[UseSession] public function solveChallenge($challengeProviderId, $challenge, $redirect_url = null) { $user = $this->userSession->getUser(); $provider = $this->twoFactorManager->getProvider($user, $challengeProviderId); diff --git a/core/Controller/WebAuthnController.php b/core/Controller/WebAuthnController.php index 81e6daf51c7f0..bd0726d2aa298 100644 --- a/core/Controller/WebAuthnController.php +++ b/core/Controller/WebAuthnController.php @@ -33,6 +33,7 @@ use OC\URLGenerator; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; +use OCP\AppFramework\Http\Attribute\UseSession; use OCP\AppFramework\Http\JSONResponse; use OCP\IRequest; use OCP\ISession; @@ -63,8 +64,8 @@ public function __construct($appName, IRequest $request, Manager $webAuthnManger /** * @NoAdminRequired * @PublicPage - * @UseSession */ + #[UseSession] public function startAuthentication(string $loginName): JSONResponse { $this->logger->debug('Starting WebAuthn login'); @@ -87,8 +88,8 @@ public function startAuthentication(string $loginName): JSONResponse { /** * @NoAdminRequired * @PublicPage - * @UseSession */ + #[UseSession] public function finishAuthentication(string $data): JSONResponse { $this->logger->debug('Validating WebAuthn login'); diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 853d97f4a2e3e..c24a7d198d7d9 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -35,6 +35,7 @@ 'OCP\\AppFramework\\Db\\QBMapper' => $baseDir . '/lib/public/AppFramework/Db/QBMapper.php', 'OCP\\AppFramework\\Db\\TTransactional' => $baseDir . '/lib/public/AppFramework/Db/TTransactional.php', 'OCP\\AppFramework\\Http' => $baseDir . '/lib/public/AppFramework/Http.php', + 'OCP\\AppFramework\\Http\\Attribute\\UseSession' => $baseDir . '/lib/public/AppFramework/Http/Attribute/UseSession.php', 'OCP\\AppFramework\\Http\\ContentSecurityPolicy' => $baseDir . '/lib/public/AppFramework/Http/ContentSecurityPolicy.php', 'OCP\\AppFramework\\Http\\DataDisplayResponse' => $baseDir . '/lib/public/AppFramework/Http/DataDisplayResponse.php', 'OCP\\AppFramework\\Http\\DataDownloadResponse' => $baseDir . '/lib/public/AppFramework/Http/DataDownloadResponse.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 455eccc45fd64..2df8ebfde50a4 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -68,6 +68,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\AppFramework\\Db\\QBMapper' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Db/QBMapper.php', 'OCP\\AppFramework\\Db\\TTransactional' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Db/TTransactional.php', 'OCP\\AppFramework\\Http' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http.php', + 'OCP\\AppFramework\\Http\\Attribute\\UseSession' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/Attribute/UseSession.php', 'OCP\\AppFramework\\Http\\ContentSecurityPolicy' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/ContentSecurityPolicy.php', 'OCP\\AppFramework\\Http\\DataDisplayResponse' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/DataDisplayResponse.php', 'OCP\\AppFramework\\Http\\DataDownloadResponse' => __DIR__ . '/../../..' . '/lib/public/AppFramework/Http/DataDownloadResponse.php', diff --git a/lib/private/AppFramework/Middleware/SessionMiddleware.php b/lib/private/AppFramework/Middleware/SessionMiddleware.php index af195df0de919..39f85915901d6 100644 --- a/lib/private/AppFramework/Middleware/SessionMiddleware.php +++ b/lib/private/AppFramework/Middleware/SessionMiddleware.php @@ -1,4 +1,7 @@ reflector->hasAnnotation('UseSession'); - if ($useSession) { + /** + * Annotation deprecated with Nextcloud 26 + */ + $hasAnnotation = $this->reflector->hasAnnotation('UseSession'); + if ($hasAnnotation) { + $this->session->reopen(); + return; + } + + $reflectionMethod = new ReflectionMethod($controller, $methodName); + $hasAttribute = !empty($reflectionMethod->getAttributes(UseSession::class)); + if ($hasAttribute) { $this->session->reopen(); } } @@ -62,10 +77,21 @@ public function beforeController($controller, $methodName) { * @return Response */ public function afterController($controller, $methodName, Response $response) { - $useSession = $this->reflector->hasAnnotation('UseSession'); - if ($useSession) { + /** + * Annotation deprecated with Nextcloud 26 + */ + $hasAnnotation = $this->reflector->hasAnnotation('UseSession'); + if ($hasAnnotation) { $this->session->close(); + return $response; } + + $reflectionMethod = new ReflectionMethod($controller, $methodName); + $hasAttribute = !empty($reflectionMethod->getAttributes(UseSession::class)); + if ($hasAttribute) { + $this->session->close(); + } + return $response; } } diff --git a/lib/public/AppFramework/Http/Attribute/UseSession.php b/lib/public/AppFramework/Http/Attribute/UseSession.php new file mode 100644 index 0000000000000..79185919defbb --- /dev/null +++ b/lib/public/AppFramework/Http/Attribute/UseSession.php @@ -0,0 +1,37 @@ + + * + * @author 2023 Christoph Wurst + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +namespace OCP\AppFramework\Http\Attribute; + +use Attribute; + +/** + * Attribute for controller methods that need to read/write PHP session data + * + * @since 26.0.0 + */ +#[Attribute] +class UseSession { +} diff --git a/tests/lib/AppFramework/Middleware/SessionMiddlewareTest.php b/tests/lib/AppFramework/Middleware/SessionMiddlewareTest.php index 9641042d1b915..09838423e856b 100644 --- a/tests/lib/AppFramework/Middleware/SessionMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/SessionMiddlewareTest.php @@ -1,4 +1,7 @@ reflector = new ControllerMethodReflector(); - $this->controller = $this->createMock(Controller::class); + $this->reflector = $this->createMock(ControllerMethodReflector::class); + $this->session = $this->createMock(ISession::class); + $this->controller = new class('app', $this->createMock(IRequest::class)) extends Controller { + /** + * @UseSession + */ + public function withAnnotation() { + } + #[UseSession] + public function withAttribute() { + } + public function without() { + } + }; + $this->middleware = new SessionMiddleware( + $this->reflector, + $this->session, + ); + } + + public function testSessionNotClosedOnBeforeController(): void { + $this->configureSessionMock(0, 1); + $this->reflector->expects(self::once()) + ->method('hasAnnotation') + ->with('UseSession') + ->willReturn(true); + + $this->middleware->beforeController($this->controller, 'withAnnotation'); } - /** - * @UseSession - */ - public function testSessionNotClosedOnBeforeController() { - $session = $this->getSessionMock(0, 1); + public function testSessionNotClosedOnBeforeControllerWithAttribute(): void { + $this->configureSessionMock(0, 1); + $this->reflector->expects(self::once()) + ->method('hasAnnotation') + ->with('UseSession') + ->willReturn(false); - $this->reflector->reflect($this, __FUNCTION__); - $middleware = new SessionMiddleware($this->reflector, $session); - $middleware->beforeController($this->controller, __FUNCTION__); + $this->middleware->beforeController($this->controller, 'withAttribute'); } - /** - * @UseSession - */ - public function testSessionClosedOnAfterController() { - $session = $this->getSessionMock(1); + public function testSessionClosedOnAfterController(): void { + $this->configureSessionMock(1); + $this->reflector->expects(self::once()) + ->method('hasAnnotation') + ->with('UseSession') + ->willReturn(true); - $this->reflector->reflect($this, __FUNCTION__); - $middleware = new SessionMiddleware($this->reflector, $session); - $middleware->afterController($this->controller, __FUNCTION__, new Response()); + $this->middleware->afterController($this->controller, 'withAnnotation', new Response()); } - /** - * @UseSession - */ - public function testSessionReopenedAndClosedOnBeforeController() { - $session = $this->getSessionMock(1, 1); + public function testSessionClosedOnAfterControllerWithAttribute(): void { + $this->configureSessionMock(1); + $this->reflector->expects(self::once()) + ->method('hasAnnotation') + ->with('UseSession') + ->willReturn(true); - $this->reflector->reflect($this, __FUNCTION__); - $middleware = new SessionMiddleware($this->reflector, $session); - $middleware->beforeController($this->controller, __FUNCTION__); - $middleware->afterController($this->controller, __FUNCTION__, new Response()); + $this->middleware->afterController($this->controller, 'withAttribute', new Response()); } - public function testSessionClosedOnBeforeController() { - $session = $this->getSessionMock(0); + public function testSessionReopenedAndClosedOnBeforeController(): void { + $this->configureSessionMock(1, 1); + $this->reflector->expects(self::exactly(2)) + ->method('hasAnnotation') + ->with('UseSession') + ->willReturn(true); - $this->reflector->reflect($this, __FUNCTION__); - $middleware = new SessionMiddleware($this->reflector, $session); - $middleware->beforeController($this->controller, __FUNCTION__); + $this->middleware->beforeController($this->controller, 'withAnnotation'); + $this->middleware->afterController($this->controller, 'withAnnotation', new Response()); } - public function testSessionNotClosedOnAfterController() { - $session = $this->getSessionMock(0); + public function testSessionReopenedAndClosedOnBeforeControllerWithAttribute(): void { + $this->configureSessionMock(1, 1); + $this->reflector->expects(self::exactly(2)) + ->method('hasAnnotation') + ->with('UseSession') + ->willReturn(false); - $this->reflector->reflect($this, __FUNCTION__); - $middleware = new SessionMiddleware($this->reflector, $session); - $middleware->afterController($this->controller, __FUNCTION__, new Response()); + $this->middleware->beforeController($this->controller, 'withAttribute'); + $this->middleware->afterController($this->controller, 'withAttribute', new Response()); } - /** - * @return mixed - */ - private function getSessionMock(int $expectedCloseCount, int $expectedReopenCount = 0) { - $session = $this->getMockBuilder('\OC\Session\Memory') - ->disableOriginalConstructor() - ->getMock(); + public function testSessionClosedOnBeforeController(): void { + $this->configureSessionMock(0); + $this->reflector->expects(self::once()) + ->method('hasAnnotation') + ->with('UseSession') + ->willReturn(false); + + $this->middleware->beforeController($this->controller, 'without'); + } + + public function testSessionNotClosedOnAfterController(): void { + $this->configureSessionMock(0); + $this->reflector->expects(self::once()) + ->method('hasAnnotation') + ->with('UseSession') + ->willReturn(false); + + $this->middleware->afterController($this->controller, 'without', new Response()); + } - $session->expects($this->exactly($expectedCloseCount)) + private function configureSessionMock(int $expectedCloseCount, int $expectedReopenCount = 0): void { + $this->session->expects($this->exactly($expectedCloseCount)) ->method('close'); - $session->expects($this->exactly($expectedReopenCount)) + $this->session->expects($this->exactly($expectedReopenCount)) ->method('reopen'); - return $session; } }