From 02313013ad9094ea0ec52f30672971ab03f4ec15 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 1 Mar 2024 18:37:47 +0100 Subject: fix(Session): avoid password confirmation on SSO SSO backends like SAML and OIDC tried a trick to suppress password confirmations as they are not possible by design. At least for SAML it was not reliable when existing user backends where used as user repositories. Now we are setting a special scope with the token, and also make sure that the scope is taken over when tokens are regenerated. Signed-off-by: Arthur Schiwon --- core/Controller/OCJSController.php | 9 ++- .../DependencyInjection/DIContainer.php | 3 +- .../Security/PasswordConfirmationMiddleware.php | 30 +++++++-- .../Token/PublicKeyTokenProvider.php | 1 + lib/private/Template/JSConfigHelper.php | 73 +++++++++++----------- lib/private/TemplateLayout.php | 4 +- lib/private/legacy/OC_User.php | 10 ++- .../PasswordConfirmationMiddlewareController.php | 4 ++ .../PasswordConfirmationMiddlewareTest.php | 60 +++++++++++++++++- 9 files changed, 148 insertions(+), 46 deletions(-) diff --git a/core/Controller/OCJSController.php b/core/Controller/OCJSController.php index fa13f21607c..4f84e551af7 100644 --- a/core/Controller/OCJSController.php +++ b/core/Controller/OCJSController.php @@ -28,6 +28,7 @@ namespace OC\Core\Controller; use bantu\IniGetWrapper\IniGetWrapper; +use OC\Authentication\Token\IProvider; use OC\CapabilitiesManager; use OC\Template\JSConfigHelper; use OCP\App\IAppManager; @@ -59,7 +60,10 @@ class OCJSController extends Controller { IniGetWrapper $iniWrapper, IURLGenerator $urlGenerator, CapabilitiesManager $capabilitiesManager, - IInitialStateService $initialStateService) { + IInitialStateService $initialStateService, + IProvider $tokenProvider, + ) { + parent::__construct($appName, $request); $this->helper = new JSConfigHelper( @@ -73,7 +77,8 @@ class OCJSController extends Controller { $iniWrapper, $urlGenerator, $capabilitiesManager, - $initialStateService + $initialStateService, + $tokenProvider ); } diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 9a9740b7bcc..4808cda67c3 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -275,7 +275,8 @@ class DIContainer extends SimpleContainer implements IAppContainer { $c->get(IControllerMethodReflector::class), $c->get(ISession::class), $c->get(IUserSession::class), - $c->get(ITimeFactory::class) + $c->get(ITimeFactory::class), + $c->get(\OC\Authentication\Token\IProvider::class), ) ); $dispatcher->registerMiddleware( diff --git a/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php b/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php index a72a7a40016..45278254762 100644 --- a/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php @@ -25,12 +25,17 @@ namespace OC\AppFramework\Middleware\Security; use OC\AppFramework\Middleware\Security\Exceptions\NotConfirmedException; use OC\AppFramework\Utility\ControllerMethodReflector; +use OC\Authentication\Exceptions\ExpiredTokenException; +use OC\Authentication\Exceptions\InvalidTokenException; +use OC\Authentication\Exceptions\WipeTokenException; +use OC\Authentication\Token\IProvider; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired; use OCP\AppFramework\Middleware; use OCP\AppFramework\Utility\ITimeFactory; use OCP\ISession; use OCP\IUserSession; +use OCP\Session\Exceptions\SessionNotAvailableException; use OCP\User\Backend\IPasswordConfirmationBackend; use ReflectionMethod; @@ -45,6 +50,7 @@ class PasswordConfirmationMiddleware extends Middleware { private $timeFactory; /** @var array */ private $excludedUserBackEnds = ['user_saml' => true, 'user_globalsiteselector' => true]; + private IProvider $tokenProvider; /** * PasswordConfirmationMiddleware constructor. @@ -55,13 +61,16 @@ class PasswordConfirmationMiddleware extends Middleware { * @param ITimeFactory $timeFactory */ public function __construct(ControllerMethodReflector $reflector, - ISession $session, - IUserSession $userSession, - ITimeFactory $timeFactory) { + ISession $session, + IUserSession $userSession, + ITimeFactory $timeFactory, + IProvider $tokenProvider, + ) { $this->reflector = $reflector; $this->session = $session; $this->userSession = $userSession; $this->timeFactory = $timeFactory; + $this->tokenProvider = $tokenProvider; } /** @@ -86,8 +95,21 @@ class PasswordConfirmationMiddleware extends Middleware { $backendClassName = $user->getBackendClassName(); } + try { + $sessionId = $this->session->getId(); + $token = $this->tokenProvider->getToken($sessionId); + } catch (SessionNotAvailableException|InvalidTokenException|WipeTokenException|ExpiredTokenException) { + // States we do not deal with here. + return; + } + $scope = $token->getScopeAsArray(); + if (isset($scope['sso-based-login']) && $scope['sso-based-login'] === true) { + // Users logging in from SSO backends cannot confirm their password by design + return; + } + $lastConfirm = (int) $this->session->get('last-password-confirm'); - // we can't check the password against a SAML backend, so skip password confirmation in this case + // TODO: confirm excludedUserBackEnds can go away and remove it if (!isset($this->excludedUserBackEnds[$backendClassName]) && $lastConfirm < ($this->timeFactory->getTime() - (30 * 60 + 15))) { // allow 15 seconds delay throw new NotConfirmedException(); } diff --git a/lib/private/Authentication/Token/PublicKeyTokenProvider.php b/lib/private/Authentication/Token/PublicKeyTokenProvider.php index bab025973b9..f64dd08fbed 100644 --- a/lib/private/Authentication/Token/PublicKeyTokenProvider.php +++ b/lib/private/Authentication/Token/PublicKeyTokenProvider.php @@ -262,6 +262,7 @@ class PublicKeyTokenProvider implements IProvider { IToken::TEMPORARY_TOKEN, $token->getRemember() ); + $newToken->setScope($token->getScopeAsArray()); $this->cacheToken($newToken); $this->cacheInvalidHash($token->getToken()); diff --git a/lib/private/Template/JSConfigHelper.php b/lib/private/Template/JSConfigHelper.php index 7b6d0a6a346..d890ac785af 100644 --- a/lib/private/Template/JSConfigHelper.php +++ b/lib/private/Template/JSConfigHelper.php @@ -34,6 +34,10 @@ declare(strict_types=1); namespace OC\Template; use bantu\IniGetWrapper\IniGetWrapper; +use OC\Authentication\Exceptions\ExpiredTokenException; +use OC\Authentication\Exceptions\InvalidTokenException; +use OC\Authentication\Exceptions\WipeTokenException; +use OC\Authentication\Token\IProvider; use OC\CapabilitiesManager; use OC\Share\Share; use OCP\App\AppPathNotFoundException; @@ -49,47 +53,29 @@ use OCP\ISession; use OCP\IURLGenerator; use OCP\ILogger; use OCP\IUser; +use OCP\Session\Exceptions\SessionNotAvailableException; use OCP\User\Backend\IPasswordConfirmationBackend; use OCP\Util; class JSConfigHelper { - protected IL10N $l; - protected Defaults $defaults; - protected IAppManager $appManager; - protected ISession $session; - protected ?IUser $currentUser; - protected IConfig $config; - protected IGroupManager $groupManager; - protected IniGetWrapper $iniWrapper; - protected IURLGenerator $urlGenerator; - protected CapabilitiesManager $capabilitiesManager; - protected IInitialStateService $initialStateService; /** @var array user back-ends excluded from password verification */ private $excludedUserBackEnds = ['user_saml' => true, 'user_globalsiteselector' => true]; - public function __construct(IL10N $l, - Defaults $defaults, - IAppManager $appManager, - ISession $session, - ?IUser $currentUser, - IConfig $config, - IGroupManager $groupManager, - IniGetWrapper $iniWrapper, - IURLGenerator $urlGenerator, - CapabilitiesManager $capabilitiesManager, - IInitialStateService $initialStateService) { - $this->l = $l; - $this->defaults = $defaults; - $this->appManager = $appManager; - $this->session = $session; - $this->currentUser = $currentUser; - $this->config = $config; - $this->groupManager = $groupManager; - $this->iniWrapper = $iniWrapper; - $this->urlGenerator = $urlGenerator; - $this->capabilitiesManager = $capabilitiesManager; - $this->initialStateService = $initialStateService; + public function __construct( + protected IL10N $l, + protected Defaults $defaults, + protected IAppManager $appManager, + protected ISession $session, + protected ?IUser $currentUser, + protected IConfig $config, + protected IGroupManager $groupManager, + protected IniGetWrapper $iniWrapper, + protected IURLGenerator $urlGenerator, + protected CapabilitiesManager $capabilitiesManager, + protected IInitialStateService $initialStateService, + protected IProvider $tokenProvider, + ) { } public function getConfig(): string { @@ -155,9 +141,13 @@ class JSConfigHelper { } if ($this->currentUser instanceof IUser) { - $lastConfirmTimestamp = $this->session->get('last-password-confirm'); - if (!is_int($lastConfirmTimestamp)) { - $lastConfirmTimestamp = 0; + if ($this->canUserValidatePassword()) { + $lastConfirmTimestamp = $this->session->get('last-password-confirm'); + if (!is_int($lastConfirmTimestamp)) { + $lastConfirmTimestamp = 0; + } + } else { + $lastConfirmTimestamp = PHP_INT_MAX; } } else { $lastConfirmTimestamp = 0; @@ -310,4 +300,15 @@ class JSConfigHelper { return $result; } + + protected function canUserValidatePassword(): bool { + try { + $token = $this->tokenProvider->getToken($this->session->getId()); + } catch (ExpiredTokenException|WipeTokenException|InvalidTokenException|SessionNotAvailableException) { + // actually we do not know, so we fall back to this statement + return true; + } + $scope = $token->getScopeAsArray(); + return !isset($scope['sso-based-login']) || $scope['sso-based-login'] === false; + } } diff --git a/lib/private/TemplateLayout.php b/lib/private/TemplateLayout.php index 52c60e58991..dec6d2186fc 100644 --- a/lib/private/TemplateLayout.php +++ b/lib/private/TemplateLayout.php @@ -43,6 +43,7 @@ namespace OC; use bantu\IniGetWrapper\IniGetWrapper; +use OC\Authentication\Token\IProvider; use OC\Search\SearchQuery; use OC\Template\CSSResourceLocator; use OC\Template\JSConfigHelper; @@ -235,7 +236,8 @@ class TemplateLayout extends \OC_Template { \OC::$server->get(IniGetWrapper::class), \OC::$server->getURLGenerator(), \OC::$server->getCapabilitiesManager(), - \OC::$server->query(IInitialStateService::class) + \OCP\Server::get(IInitialStateService::class), + \OCP\Server::get(IProvider::class), ); $config = $jsConfigHelper->getConfig(); if (\OC::$server->getContentSecurityPolicyNonceManager()->browserSupportsCspV3()) { diff --git a/lib/private/legacy/OC_User.php b/lib/private/legacy/OC_User.php index caa4f5dca65..24ffaa3b3aa 100644 --- a/lib/private/legacy/OC_User.php +++ b/lib/private/legacy/OC_User.php @@ -35,7 +35,7 @@ * along with this program. If not, see * */ - +use OC\Authentication\Token\IProvider; use OC\User\LoginException; use OCP\EventDispatcher\IEventDispatcher; use OCP\ILogger; @@ -193,6 +193,14 @@ class OC_User { $userSession->createSessionToken($request, $uid, $uid, $password); $userSession->createRememberMeToken($userSession->getUser()); + + if (empty($password)) { + $tokenProvider = \OC::$server->get(IProvider::class); + $token = $tokenProvider->getToken($userSession->getSession()->getId()); + $token->setScope(['sso-based-login' => true]); + $tokenProvider->updateToken($token); + } + // setup the filesystem OC_Util::setupFS($uid); // first call the post_login hooks, the login-process needs to be diff --git a/tests/lib/AppFramework/Middleware/Security/Mock/PasswordConfirmationMiddlewareController.php b/tests/lib/AppFramework/Middleware/Security/Mock/PasswordConfirmationMiddlewareController.php index 5b83575f711..941906d8bb6 100644 --- a/tests/lib/AppFramework/Middleware/Security/Mock/PasswordConfirmationMiddlewareController.php +++ b/tests/lib/AppFramework/Middleware/Security/Mock/PasswordConfirmationMiddlewareController.php @@ -46,4 +46,8 @@ class PasswordConfirmationMiddlewareController extends \OCP\AppFramework\Control #[PasswordConfirmationRequired] public function testAttribute() { } + + #[PasswordConfirmationRequired] + public function testSSO() { + } } diff --git a/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php index 3752259c61b..252f50e8147 100644 --- a/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php @@ -26,7 +26,9 @@ namespace Test\AppFramework\Middleware\Security; use OC\AppFramework\Middleware\Security\Exceptions\NotConfirmedException; use OC\AppFramework\Middleware\Security\PasswordConfirmationMiddleware; use OC\AppFramework\Utility\ControllerMethodReflector; +use OC\Authentication\Token\IProvider; use OCP\AppFramework\Utility\ITimeFactory; +use OC\Authentication\Token\IToken; use OCP\IRequest; use OCP\ISession; use OCP\IUser; @@ -49,6 +51,7 @@ class PasswordConfirmationMiddlewareTest extends TestCase { private $controller; /** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */ private $timeFactory; + private IProvider|\PHPUnit\Framework\MockObject\MockObject $tokenProvider; protected function setUp(): void { $this->reflector = new ControllerMethodReflector(); @@ -56,6 +59,7 @@ class PasswordConfirmationMiddlewareTest extends TestCase { $this->userSession = $this->createMock(IUserSession::class); $this->user = $this->createMock(IUser::class); $this->timeFactory = $this->createMock(ITimeFactory::class); + $this->tokenProvider = $this->createMock(IProvider::class); $this->controller = new PasswordConfirmationMiddlewareController( 'test', $this->createMock(IRequest::class) @@ -65,7 +69,8 @@ class PasswordConfirmationMiddlewareTest extends TestCase { $this->reflector, $this->session, $this->userSession, - $this->timeFactory + $this->timeFactory, + $this->tokenProvider, ); } @@ -107,6 +112,13 @@ class PasswordConfirmationMiddlewareTest extends TestCase { $this->timeFactory->method('getTime') ->willReturn($currentTime); + $token = $this->createMock(IToken::class); + $token->method('getScopeAsArray') + ->willReturn([]); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->willReturn($token); + $thrown = false; try { $this->middleware->beforeController($this->controller, __FUNCTION__); @@ -135,6 +147,13 @@ class PasswordConfirmationMiddlewareTest extends TestCase { $this->timeFactory->method('getTime') ->willReturn($currentTime); + $token = $this->createMock(IToken::class); + $token->method('getScopeAsArray') + ->willReturn([]); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->willReturn($token); + $thrown = false; try { $this->middleware->beforeController($this->controller, __FUNCTION__); @@ -145,6 +164,8 @@ class PasswordConfirmationMiddlewareTest extends TestCase { $this->assertSame($exception, $thrown); } + + public function dataProvider() { return [ ['foo', 2000, 4000, true], @@ -155,4 +176,41 @@ class PasswordConfirmationMiddlewareTest extends TestCase { ['foo', 2000, 3816, true], ]; } + + public function testSSO() { + static $sessionId = 'mySession1d'; + + $this->reflector->reflect($this->controller, __FUNCTION__); + + $this->user->method('getBackendClassName') + ->willReturn('fictional_backend'); + $this->userSession->method('getUser') + ->willReturn($this->user); + + $this->session->method('get') + ->with('last-password-confirm') + ->willReturn(0); + $this->session->method('getId') + ->willReturn($sessionId); + + $this->timeFactory->method('getTime') + ->willReturn(9876); + + $token = $this->createMock(IToken::class); + $token->method('getScopeAsArray') + ->willReturn(['sso-based-login' => true]); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->with($sessionId) + ->willReturn($token); + + $thrown = false; + try { + $this->middleware->beforeController($this->controller, __FUNCTION__); + } catch (NotConfirmedException) { + $thrown = true; + } + + $this->assertSame(false, $thrown); + } } -- cgit v1.2.3 From 3b840dfb795b7b3037b4193da451138df297c5c2 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 12 Jun 2024 11:05:43 +0200 Subject: fix(Token): make new scope future compatible - "password-unconfirmable" is the effective name for 30, but a draft name was backported. Signed-off-by: Arthur Schiwon --- .../AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php | 2 +- lib/private/Template/JSConfigHelper.php | 2 +- lib/private/legacy/OC_User.php | 2 +- .../Middleware/Security/PasswordConfirmationMiddlewareTest.php | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php b/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php index 45278254762..f41e61b4a18 100644 --- a/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php @@ -103,7 +103,7 @@ class PasswordConfirmationMiddleware extends Middleware { return; } $scope = $token->getScopeAsArray(); - if (isset($scope['sso-based-login']) && $scope['sso-based-login'] === true) { + if (isset($scope['password-unconfirmable']) && $scope['password-unconfirmable'] === true) { // Users logging in from SSO backends cannot confirm their password by design return; } diff --git a/lib/private/Template/JSConfigHelper.php b/lib/private/Template/JSConfigHelper.php index d890ac785af..6942fdeebcb 100644 --- a/lib/private/Template/JSConfigHelper.php +++ b/lib/private/Template/JSConfigHelper.php @@ -309,6 +309,6 @@ class JSConfigHelper { return true; } $scope = $token->getScopeAsArray(); - return !isset($scope['sso-based-login']) || $scope['sso-based-login'] === false; + return !isset($scope['password-unconfirmable']) || $scope['password-unconfirmable'] === false; } } diff --git a/lib/private/legacy/OC_User.php b/lib/private/legacy/OC_User.php index 24ffaa3b3aa..ec0b7e69c8a 100644 --- a/lib/private/legacy/OC_User.php +++ b/lib/private/legacy/OC_User.php @@ -197,7 +197,7 @@ class OC_User { if (empty($password)) { $tokenProvider = \OC::$server->get(IProvider::class); $token = $tokenProvider->getToken($userSession->getSession()->getId()); - $token->setScope(['sso-based-login' => true]); + $token->setScope(['password-unconfirmable' => true]); $tokenProvider->updateToken($token); } diff --git a/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php index 252f50e8147..dd233dfa079 100644 --- a/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php @@ -198,7 +198,7 @@ class PasswordConfirmationMiddlewareTest extends TestCase { $token = $this->createMock(IToken::class); $token->method('getScopeAsArray') - ->willReturn(['sso-based-login' => true]); + ->willReturn(['password-unconfirmable' => true]); $this->tokenProvider->expects($this->once()) ->method('getToken') ->with($sessionId) -- cgit v1.2.3 From 495ccc9becd7a64fdac5ef7b8281b9f8ba8bfa80 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 12 Jun 2024 14:58:27 +0200 Subject: style(PHP): remove unacceptable empty lines Signed-off-by: Arthur Schiwon --- core/Controller/OCJSController.php | 1 - lib/private/Template/JSConfigHelper.php | 1 - 2 files changed, 2 deletions(-) diff --git a/core/Controller/OCJSController.php b/core/Controller/OCJSController.php index 4f84e551af7..af0d4689efb 100644 --- a/core/Controller/OCJSController.php +++ b/core/Controller/OCJSController.php @@ -63,7 +63,6 @@ class OCJSController extends Controller { IInitialStateService $initialStateService, IProvider $tokenProvider, ) { - parent::__construct($appName, $request); $this->helper = new JSConfigHelper( diff --git a/lib/private/Template/JSConfigHelper.php b/lib/private/Template/JSConfigHelper.php index 6942fdeebcb..e744b5362ce 100644 --- a/lib/private/Template/JSConfigHelper.php +++ b/lib/private/Template/JSConfigHelper.php @@ -58,7 +58,6 @@ use OCP\User\Backend\IPasswordConfirmationBackend; use OCP\Util; class JSConfigHelper { - /** @var array user back-ends excluded from password verification */ private $excludedUserBackEnds = ['user_saml' => true, 'user_globalsiteselector' => true]; -- cgit v1.2.3