diff options
author | Christoph Wurst <christoph@winzerhof-wurst.at> | 2023-01-25 19:26:21 +0100 |
---|---|---|
committer | Christoph Wurst <christoph@winzerhof-wurst.at> | 2023-01-27 09:40:35 +0100 |
commit | 20e00cdf17f45f811135fe5fb61c133ce9021144 (patch) | |
tree | b9bb2d0c10ede9f4532bddeebcbdb3c1d5febc98 | |
parent | be1de30a4f6b37da7c710ac55489e4938c4ded91 (diff) | |
download | nextcloud-server-20e00cdf17f45f811135fe5fb61c133ce9021144.tar.gz nextcloud-server-20e00cdf17f45f811135fe5fb61c133ce9021144.zip |
feat(app-framework): Add UseSession attribute to replace annotation
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
-rw-r--r-- | core/Controller/ClientFlowLoginController.php | 7 | ||||
-rw-r--r-- | core/Controller/ClientFlowLoginV2Controller.php | 9 | ||||
-rw-r--r-- | core/Controller/LoginController.php | 9 | ||||
-rw-r--r-- | core/Controller/TwoFactorChallengeController.php | 5 | ||||
-rw-r--r-- | core/Controller/WebAuthnController.php | 5 | ||||
-rw-r--r-- | lib/composer/composer/autoload_classmap.php | 1 | ||||
-rw-r--r-- | lib/composer/composer/autoload_static.php | 1 | ||||
-rw-r--r-- | lib/private/AppFramework/Middleware/SessionMiddleware.php | 34 | ||||
-rw-r--r-- | lib/public/AppFramework/Http/Attribute/UseSession.php | 37 | ||||
-rw-r--r-- | tests/lib/AppFramework/Middleware/SessionMiddlewareTest.php | 153 |
10 files changed, 189 insertions, 72 deletions
diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 4c262714fe6..85a793bd92b 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -41,6 +41,7 @@ use OCA\OAuth2\Db\AccessTokenMapper; 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 @@ class ClientFlowLoginController extends Controller { /** * @PublicPage * @NoCSRFRequired - * @UseSession */ + #[UseSession] public function showAuthPickerPage(string $clientIdentifier = '', string $user = '', int $direct = 0): StandaloneTemplateResponse { $clientName = $this->getClientName(); $client = null; @@ -193,8 +194,8 @@ class ClientFlowLoginController extends Controller { * @NoAdminRequired * @NoCSRFRequired * @NoSameSiteCookieRequired - * @UseSession */ + #[UseSession] public function grantPage(string $stateToken = '', string $clientIdentifier = '', int $direct = 0): StandaloneTemplateResponse { @@ -243,10 +244,10 @@ class ClientFlowLoginController extends Controller { /** * @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 4df94a28d6a..d476b0cdc03 100644 --- a/core/Controller/ClientFlowLoginV2Controller.php +++ b/core/Controller/ClientFlowLoginV2Controller.php @@ -33,6 +33,7 @@ use OC\Core\Exception\LoginFlowV2NotFoundException; 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 @@ class ClientFlowLoginV2Controller extends Controller { /** * @NoCSRFRequired * @PublicPage - * @UseSession */ + #[UseSession] public function landing(string $token, $user = ''): Response { if (!$this->loginFlowV2Service->startLoginFlow($token)) { return $this->loginTokenForbiddenResponse(); @@ -114,8 +115,8 @@ class ClientFlowLoginV2Controller extends Controller { /** * @NoCSRFRequired * @PublicPage - * @UseSession */ + #[UseSession] public function showAuthPickerPage($user = ''): StandaloneTemplateResponse { try { $flow = $this->getFlowByLoginToken(); @@ -145,10 +146,10 @@ class ClientFlowLoginV2Controller extends Controller { /** * @NoAdminRequired - * @UseSession * @NoCSRFRequired * @NoSameSiteCookieRequired */ + #[UseSession] public function grantPage(string $stateToken): StandaloneTemplateResponse { if (!$this->isValidStateToken($stateToken)) { return $this->stateTokenForbiddenResponse(); @@ -222,8 +223,8 @@ class ClientFlowLoginV2Controller extends Controller { /** * @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 8fd994ae648..b93c6002ed5 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -43,6 +43,7 @@ use OC\User\Session; 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 @@ class LoginController extends Controller { /** * @NoAdminRequired - * @UseSession * * @return RedirectResponse */ + #[UseSession] public function logout() { $loginToken = $this->request->getCookie('nc_token'); if (!is_null($loginToken)) { @@ -134,13 +135,13 @@ class LoginController extends Controller { /** * @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 @@ class LoginController extends Controller { /** * @PublicPage - * @UseSession * @NoCSRFRequired * @BruteForceProtection(action=login) * * @return RedirectResponse */ + #[UseSession] public function tryLogin(Chain $loginChain, string $user, string $password, @@ -368,12 +369,12 @@ class LoginController extends Controller { /** * @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 deebeb21d32..7a57d5eeb1a 100644 --- a/core/Controller/TwoFactorChallengeController.php +++ b/core/Controller/TwoFactorChallengeController.php @@ -28,6 +28,7 @@ namespace OC\Core\Controller; 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 @@ class TwoFactorChallengeController extends Controller { /** * @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 @@ class TwoFactorChallengeController extends Controller { /** * @NoAdminRequired * @NoCSRFRequired - * @UseSession * @TwoFactorSetUpDoneRequired * * @UserRateThrottle(limit=5, period=100) @@ -171,6 +171,7 @@ class TwoFactorChallengeController extends Controller { * @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 81e6daf51c7..bd0726d2aa2 100644 --- a/core/Controller/WebAuthnController.php +++ b/core/Controller/WebAuthnController.php @@ -33,6 +33,7 @@ use OC\Authentication\WebAuthn\Manager; 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 @@ class WebAuthnController extends Controller { /** * @NoAdminRequired * @PublicPage - * @UseSession */ + #[UseSession] public function startAuthentication(string $loginName): JSONResponse { $this->logger->debug('Starting WebAuthn login'); @@ -87,8 +88,8 @@ class WebAuthnController extends Controller { /** * @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 853d97f4a2e..c24a7d198d7 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -35,6 +35,7 @@ return array( '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 455eccc45fd..2df8ebfde50 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 af195df0de9..39f85915901 100644 --- a/lib/private/AppFramework/Middleware/SessionMiddleware.php +++ b/lib/private/AppFramework/Middleware/SessionMiddleware.php @@ -1,4 +1,7 @@ <?php + +declare(strict_types=1); + /** * @copyright Copyright (c) 2016, ownCloud, Inc. * @@ -27,9 +30,11 @@ namespace OC\AppFramework\Middleware; use OC\AppFramework\Utility\ControllerMethodReflector; use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\Attribute\UseSession; use OCP\AppFramework\Http\Response; use OCP\AppFramework\Middleware; use OCP\ISession; +use ReflectionMethod; class SessionMiddleware extends Middleware { /** @var ControllerMethodReflector */ @@ -49,8 +54,18 @@ class SessionMiddleware extends Middleware { * @param string $methodName */ public function beforeController($controller, $methodName) { - $useSession = $this->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 @@ class SessionMiddleware extends Middleware { * @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 00000000000..79185919def --- /dev/null +++ b/lib/public/AppFramework/Http/Attribute/UseSession.php @@ -0,0 +1,37 @@ +<?php + +declare(strict_types=1); + +/* + * @copyright 2023 Christoph Wurst <christoph@winzerhof-wurst.at> + * + * @author 2023 Christoph Wurst <christoph@winzerhof-wurst.at> + * + * @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 <http://www.gnu.org/licenses/>. + */ + +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 9641042d1b9..09838423e85 100644 --- a/tests/lib/AppFramework/Middleware/SessionMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/SessionMiddlewareTest.php @@ -1,4 +1,7 @@ <?php + +declare(strict_types=1); + /** * ownCloud - App Framework * @@ -14,84 +17,128 @@ namespace Test\AppFramework\Middleware; use OC\AppFramework\Middleware\SessionMiddleware; use OC\AppFramework\Utility\ControllerMethodReflector; use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\Attribute\UseSession; use OCP\AppFramework\Http\Response; +use OCP\IRequest; +use OCP\ISession; +use PHPUnit\Framework\MockObject\MockObject; +use Test\TestCase; -class SessionMiddlewareTest extends \Test\TestCase { - /** @var ControllerMethodReflector */ - private $reflector; - - /** @var Controller */ - private $controller; +class SessionMiddlewareTest extends TestCase { + private ControllerMethodReflector|MockObject $reflector; + private ISession|MockObject $session; + private Controller $controller; + private SessionMiddleware $middleware; protected function setUp(): void { parent::setUp(); - $this->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; } } |