Skip to content

Commit 9c4d59e

Browse files
committed
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 <[email protected]>
1 parent df1cd1b commit 9c4d59e

File tree

8 files changed

+130
-42
lines changed

8 files changed

+130
-42
lines changed

core/Controller/OCJSController.php

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
namespace OC\Core\Controller;
3030

3131
use bantu\IniGetWrapper\IniGetWrapper;
32+
use OC\Authentication\Token\IProvider;
3233
use OC\CapabilitiesManager;
3334
use OC\Template\JSConfigHelper;
3435
use OCP\App\IAppManager;
@@ -65,6 +66,7 @@ public function __construct(
6566
IURLGenerator $urlGenerator,
6667
CapabilitiesManager $capabilitiesManager,
6768
IInitialStateService $initialStateService,
69+
IProvider $tokenProvider,
6870
) {
6971
parent::__construct($appName, $request);
7072

@@ -79,7 +81,8 @@ public function __construct(
7981
$iniWrapper,
8082
$urlGenerator,
8183
$capabilitiesManager,
82-
$initialStateService
84+
$initialStateService,
85+
$tokenProvider
8386
);
8487
}
8588

lib/private/AppFramework/DependencyInjection/DIContainer.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,8 @@ public function __construct(string $appName, array $urlParams = [], ServerContai
276276
$c->get(IControllerMethodReflector::class),
277277
$c->get(ISession::class),
278278
$c->get(IUserSession::class),
279-
$c->get(ITimeFactory::class)
279+
$c->get(ITimeFactory::class),
280+
$c->get(\OC\Authentication\Token\IProvider::class),
280281
)
281282
);
282283
$dispatcher->registerMiddleware(

lib/private/AppFramework/Middleware/Security/PasswordConfirmationMiddleware.php

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,17 @@
2525

2626
use OC\AppFramework\Middleware\Security\Exceptions\NotConfirmedException;
2727
use OC\AppFramework\Utility\ControllerMethodReflector;
28+
use OC\Authentication\Token\IProvider;
2829
use OCP\AppFramework\Controller;
2930
use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired;
3031
use OCP\AppFramework\Middleware;
3132
use OCP\AppFramework\Utility\ITimeFactory;
33+
use OCP\Authentication\Exceptions\ExpiredTokenException;
34+
use OCP\Authentication\Exceptions\InvalidTokenException;
35+
use OCP\Authentication\Exceptions\WipeTokenException;
3236
use OCP\ISession;
3337
use OCP\IUserSession;
38+
use OCP\Session\Exceptions\SessionNotAvailableException;
3439
use OCP\User\Backend\IPasswordConfirmationBackend;
3540
use ReflectionMethod;
3641

@@ -45,6 +50,7 @@ class PasswordConfirmationMiddleware extends Middleware {
4550
private $timeFactory;
4651
/** @var array */
4752
private $excludedUserBackEnds = ['user_saml' => true, 'user_globalsiteselector' => true];
53+
private IProvider $tokenProvider;
4854

4955
/**
5056
* PasswordConfirmationMiddleware constructor.
@@ -57,11 +63,14 @@ class PasswordConfirmationMiddleware extends Middleware {
5763
public function __construct(ControllerMethodReflector $reflector,
5864
ISession $session,
5965
IUserSession $userSession,
60-
ITimeFactory $timeFactory) {
66+
ITimeFactory $timeFactory,
67+
IProvider $tokenProvider,
68+
) {
6169
$this->reflector = $reflector;
6270
$this->session = $session;
6371
$this->userSession = $userSession;
6472
$this->timeFactory = $timeFactory;
73+
$this->tokenProvider = $tokenProvider;
6574
}
6675

6776
/**
@@ -86,8 +95,21 @@ public function beforeController($controller, $methodName) {
8695
$backendClassName = $user->getBackendClassName();
8796
}
8897

98+
try {
99+
$sessionId = $this->session->getId();
100+
$token = $this->tokenProvider->getToken($sessionId);
101+
} catch (SessionNotAvailableException|InvalidTokenException|WipeTokenException|ExpiredTokenException) {
102+
// States we do not deal with here.
103+
return;
104+
}
105+
$scope = $token->getScopeAsArray();
106+
if (isset($scope['sso-based-login']) && $scope['sso-based-login'] === true) {
107+
// Users logging in from SSO backends cannot confirm their password by design
108+
return;
109+
}
110+
89111
$lastConfirm = (int) $this->session->get('last-password-confirm');
90-
// we can't check the password against a SAML backend, so skip password confirmation in this case
112+
// TODO: confirm excludedUserBackEnds can go away and remove it
91113
if (!isset($this->excludedUserBackEnds[$backendClassName]) && $lastConfirm < ($this->timeFactory->getTime() - (30 * 60 + 15))) { // allow 15 seconds delay
92114
throw new NotConfirmedException();
93115
}

lib/private/Authentication/Token/PublicKeyTokenProvider.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,7 @@ public function renewSessionToken(string $oldSessionId, string $sessionId): OCPI
265265
OCPIToken::TEMPORARY_TOKEN,
266266
$token->getRemember()
267267
);
268+
$newToken->setScope($token->getScopeAsArray());
268269
$this->cacheToken($newToken);
269270

270271
$this->cacheInvalidHash($token->getToken());

lib/private/Template/JSConfigHelper.php

Lines changed: 28 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
namespace OC\Template;
3535

3636
use bantu\IniGetWrapper\IniGetWrapper;
37+
use OC\Authentication\Token\IProvider;
3738
use OC\CapabilitiesManager;
3839
use OC\Share\Share;
3940
use OCP\App\AppPathNotFoundException;
@@ -53,43 +54,24 @@
5354
use OCP\Util;
5455

5556
class JSConfigHelper {
56-
protected IL10N $l;
57-
protected Defaults $defaults;
58-
protected IAppManager $appManager;
59-
protected ISession $session;
60-
protected ?IUser $currentUser;
61-
protected IConfig $config;
62-
protected IGroupManager $groupManager;
63-
protected IniGetWrapper $iniWrapper;
64-
protected IURLGenerator $urlGenerator;
65-
protected CapabilitiesManager $capabilitiesManager;
66-
protected IInitialStateService $initialStateService;
6757

6858
/** @var array user back-ends excluded from password verification */
6959
private $excludedUserBackEnds = ['user_saml' => true, 'user_globalsiteselector' => true];
7060

71-
public function __construct(IL10N $l,
72-
Defaults $defaults,
73-
IAppManager $appManager,
74-
ISession $session,
75-
?IUser $currentUser,
76-
IConfig $config,
77-
IGroupManager $groupManager,
78-
IniGetWrapper $iniWrapper,
79-
IURLGenerator $urlGenerator,
80-
CapabilitiesManager $capabilitiesManager,
81-
IInitialStateService $initialStateService) {
82-
$this->l = $l;
83-
$this->defaults = $defaults;
84-
$this->appManager = $appManager;
85-
$this->session = $session;
86-
$this->currentUser = $currentUser;
87-
$this->config = $config;
88-
$this->groupManager = $groupManager;
89-
$this->iniWrapper = $iniWrapper;
90-
$this->urlGenerator = $urlGenerator;
91-
$this->capabilitiesManager = $capabilitiesManager;
92-
$this->initialStateService = $initialStateService;
61+
public function __construct(
62+
protected IL10N $l,
63+
protected Defaults $defaults,
64+
protected IAppManager $appManager,
65+
protected ISession $session,
66+
protected ?IUser $currentUser,
67+
protected IConfig $config,
68+
protected IGroupManager $groupManager,
69+
protected IniGetWrapper $iniWrapper,
70+
protected IURLGenerator $urlGenerator,
71+
protected CapabilitiesManager $capabilitiesManager,
72+
protected IInitialStateService $initialStateService,
73+
protected IProvider $tokenProvider,
74+
) {
9375
}
9476

9577
public function getConfig(): string {
@@ -155,9 +137,13 @@ public function getConfig(): string {
155137
}
156138

157139
if ($this->currentUser instanceof IUser) {
158-
$lastConfirmTimestamp = $this->session->get('last-password-confirm');
159-
if (!is_int($lastConfirmTimestamp)) {
160-
$lastConfirmTimestamp = 0;
140+
if ($this->isUserLoggedInViaSSO()) {
141+
$lastConfirmTimestamp = PHP_INT_MAX;
142+
} else {
143+
$lastConfirmTimestamp = $this->session->get('last-password-confirm');
144+
if (!is_int($lastConfirmTimestamp)) {
145+
$lastConfirmTimestamp = 0;
146+
}
161147
}
162148
} else {
163149
$lastConfirmTimestamp = 0;
@@ -311,4 +297,10 @@ public function getConfig(): string {
311297

312298
return $result;
313299
}
300+
301+
protected function isUserLoggedInViaSSO(): bool {
302+
$token = $this->tokenProvider->getToken($this->session->getId());
303+
$scope = $token->getScopeAsArray();
304+
return isset($scope['sso-based-login']) && $scope['sso-based-login'] === true;
305+
}
314306
}

lib/private/TemplateLayout.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
namespace OC;
4444

4545
use bantu\IniGetWrapper\IniGetWrapper;
46+
use OC\Authentication\Token\IProvider;
4647
use OC\Search\SearchQuery;
4748
use OC\Template\CSSResourceLocator;
4849
use OC\Template\JSConfigHelper;
@@ -259,7 +260,8 @@ public function __construct($renderAs, $appId = '') {
259260
\OC::$server->get(IniGetWrapper::class),
260261
\OC::$server->getURLGenerator(),
261262
\OC::$server->getCapabilitiesManager(),
262-
\OCP\Server::get(IInitialStateService::class)
263+
\OCP\Server::get(IInitialStateService::class),
264+
\OCP\Server::get(IProvider::class),
263265
);
264266
$config = $jsConfigHelper->getConfig();
265267
if (\OC::$server->getContentSecurityPolicyNonceManager()->browserSupportsCspV3()) {

lib/private/legacy/OC_User.php

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
*
3737
*/
3838

39+
use OC\Authentication\Token\IProvider;
3940
use OC\User\LoginException;
4041
use OCP\EventDispatcher\IEventDispatcher;
4142
use OCP\IGroupManager;
@@ -196,6 +197,14 @@ public static function loginWithApache(\OCP\Authentication\IApacheBackend $backe
196197

197198
$userSession->createSessionToken($request, $uid, $uid, $password);
198199
$userSession->createRememberMeToken($userSession->getUser());
200+
201+
if (empty($password)) {
202+
$tokenProvider = \OC::$server->get(IProvider::class);
203+
$token = $tokenProvider->getToken($userSession->getSession()->getId());
204+
$token->setScope(['sso-based-login' => true]);
205+
$tokenProvider->updateToken($token);
206+
}
207+
199208
// setup the filesystem
200209
OC_Util::setupFS($uid);
201210
// first call the post_login hooks, the login-process needs to be

tests/lib/AppFramework/Middleware/Security/PasswordConfirmationMiddlewareTest.php

Lines changed: 59 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@
2727
use OC\AppFramework\Middleware\Security\PasswordConfirmationMiddleware;
2828
use OC\AppFramework\Utility\ControllerMethodReflector;
2929
use OCP\AppFramework\Utility\ITimeFactory;
30+
use OCP\Authentication\Token\IProvider;
31+
use OCP\Authentication\Token\IToken;
3032
use OCP\IRequest;
3133
use OCP\ISession;
3234
use OCP\IUser;
@@ -49,13 +51,15 @@ class PasswordConfirmationMiddlewareTest extends TestCase {
4951
private $controller;
5052
/** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */
5153
private $timeFactory;
54+
private IProvider|\PHPUnit\Framework\MockObject\MockObject $tokenProvider;
5255

5356
protected function setUp(): void {
5457
$this->reflector = new ControllerMethodReflector();
5558
$this->session = $this->createMock(ISession::class);
5659
$this->userSession = $this->createMock(IUserSession::class);
5760
$this->user = $this->createMock(IUser::class);
5861
$this->timeFactory = $this->createMock(ITimeFactory::class);
62+
$this->tokenProvider = $this->createMock(IProvider::class);
5963
$this->controller = new PasswordConfirmationMiddlewareController(
6064
'test',
6165
$this->createMock(IRequest::class)
@@ -65,7 +69,8 @@ protected function setUp(): void {
6569
$this->reflector,
6670
$this->session,
6771
$this->userSession,
68-
$this->timeFactory
72+
$this->timeFactory,
73+
$this->tokenProvider,
6974
);
7075
}
7176

@@ -107,6 +112,13 @@ public function testAnnotation($backend, $lastConfirm, $currentTime, $exception)
107112
$this->timeFactory->method('getTime')
108113
->willReturn($currentTime);
109114

115+
$token = $this->createMock(IToken::class);
116+
$token->method('getScopeAsArray')
117+
->willReturn([]);
118+
$this->tokenProvider->expects($this->once())
119+
->method('getToken')
120+
->willReturn($token);
121+
110122
$thrown = false;
111123
try {
112124
$this->middleware->beforeController($this->controller, __FUNCTION__);
@@ -135,6 +147,13 @@ public function testAttribute($backend, $lastConfirm, $currentTime, $exception)
135147
$this->timeFactory->method('getTime')
136148
->willReturn($currentTime);
137149

150+
$token = $this->createMock(IToken::class);
151+
$token->method('getScopeAsArray')
152+
->willReturn([]);
153+
$this->tokenProvider->expects($this->once())
154+
->method('getToken')
155+
->willReturn($token);
156+
138157
$thrown = false;
139158
try {
140159
$this->middleware->beforeController($this->controller, __FUNCTION__);
@@ -145,6 +164,8 @@ public function testAttribute($backend, $lastConfirm, $currentTime, $exception)
145164
$this->assertSame($exception, $thrown);
146165
}
147166

167+
168+
148169
public function dataProvider() {
149170
return [
150171
['foo', 2000, 4000, true],
@@ -155,4 +176,41 @@ public function dataProvider() {
155176
['foo', 2000, 3816, true],
156177
];
157178
}
179+
180+
public function testSSO() {
181+
static $sessionId = 'mySession1d';
182+
183+
$this->reflector->reflect($this->controller, __FUNCTION__);
184+
185+
$this->user->method('getBackendClassName')
186+
->willReturn('fictional_backend');
187+
$this->userSession->method('getUser')
188+
->willReturn($this->user);
189+
190+
$this->session->method('get')
191+
->with('last-password-confirm')
192+
->willReturn(0);
193+
$this->session->method('getId')
194+
->willReturn($sessionId);
195+
196+
$this->timeFactory->method('getTime')
197+
->willReturn(9876);
198+
199+
$token = $this->createMock(IToken::class);
200+
$token->method('getScopeAsArray')
201+
->willReturn(['sso-based-login' => true]);
202+
$this->tokenProvider->expects($this->once())
203+
->method('getToken')
204+
->with($sessionId)
205+
->willReturn($token);
206+
207+
$thrown = false;
208+
try {
209+
$this->middleware->beforeController($this->controller, __FUNCTION__);
210+
} catch (NotConfirmedException) {
211+
$thrown = true;
212+
}
213+
214+
$this->assertSame(false, $thrown);
215+
}
158216
}

0 commit comments

Comments
 (0)