Skip to content

Commit 98b5cdc

Browse files
blizzzblizzz
authored
Merge pull request #43942 from nextcloud/fix/43612/avoid-pwd-confirm-sso
fix(Session): avoid password confirmation on SSO
2 parents ef01dc7 + f6d6efe commit 98b5cdc

File tree

18 files changed

+183
-65
lines changed

18 files changed

+183
-65
lines changed

‎apps/settings/lib/Controller/AuthSettingsController.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -217,8 +217,8 @@ public function update($id, array $scope, string $name) {
217217
$currentName = $token->getName();
218218

219219
if ($scope !== $token->getScopeAsArray()) {
220-
$token->setScope(['filesystem' => $scope['filesystem']]);
221-
$this->publishActivity($scope['filesystem'] ? Provider::APP_TOKEN_FILESYSTEM_GRANTED : Provider::APP_TOKEN_FILESYSTEM_REVOKED, $token->getId(), ['name' => $currentName]);
220+
$token->setScope([IToken::SCOPE_FILESYSTEM => $scope[IToken::SCOPE_FILESYSTEM]]);
221+
$this->publishActivity($scope[IToken::SCOPE_FILESYSTEM] ? Provider::APP_TOKEN_FILESYSTEM_GRANTED : Provider::APP_TOKEN_FILESYSTEM_REVOKED, $token->getId(), ['name' => $currentName]);
222222
}
223223

224224
if (mb_strlen($name) > 128) {

‎apps/settings/tests/Controller/AuthSettingsControllerTest.php

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ public function testUpdateRename(string $name, string $newName): void {
244244

245245
$token->expects($this->once())
246246
->method('getScopeAsArray')
247-
->willReturn(['filesystem' => true]);
247+
->willReturn([IToken::SCOPE_FILESYSTEM => true]);
248248

249249
$token->expects($this->once())
250250
->method('setName')
@@ -254,7 +254,7 @@ public function testUpdateRename(string $name, string $newName): void {
254254
->method('updateToken')
255255
->with($this->equalTo($token));
256256

257-
$this->assertSame([], $this->controller->update($tokenId, ['filesystem' => true], $newName));
257+
$this->assertSame([], $this->controller->update($tokenId, [IToken::SCOPE_FILESYSTEM => true], $newName));
258258
}
259259

260260
public function dataUpdateFilesystemScope(): array {
@@ -287,17 +287,17 @@ public function testUpdateFilesystemScope(bool $filesystem, bool $newFilesystem)
287287

288288
$token->expects($this->once())
289289
->method('getScopeAsArray')
290-
->willReturn(['filesystem' => $filesystem]);
290+
->willReturn([IToken::SCOPE_FILESYSTEM => $filesystem]);
291291

292292
$token->expects($this->once())
293293
->method('setScope')
294-
->with($this->equalTo(['filesystem' => $newFilesystem]));
294+
->with($this->equalTo([IToken::SCOPE_FILESYSTEM => $newFilesystem]));
295295

296296
$this->tokenProvider->expects($this->once())
297297
->method('updateToken')
298298
->with($this->equalTo($token));
299299

300-
$this->assertSame([], $this->controller->update($tokenId, ['filesystem' => $newFilesystem], 'App password'));
300+
$this->assertSame([], $this->controller->update($tokenId, [IToken::SCOPE_FILESYSTEM => $newFilesystem], 'App password'));
301301
}
302302

303303
public function testUpdateNoChange(): void {
@@ -316,7 +316,7 @@ public function testUpdateNoChange(): void {
316316

317317
$token->expects($this->once())
318318
->method('getScopeAsArray')
319-
->willReturn(['filesystem' => true]);
319+
->willReturn([IToken::SCOPE_FILESYSTEM => true]);
320320

321321
$token->expects($this->never())
322322
->method('setName');
@@ -328,7 +328,7 @@ public function testUpdateNoChange(): void {
328328
->method('updateToken')
329329
->with($this->equalTo($token));
330330

331-
$this->assertSame([], $this->controller->update($tokenId, ['filesystem' => true], 'App password'));
331+
$this->assertSame([], $this->controller->update($tokenId, [IToken::SCOPE_FILESYSTEM => true], 'App password'));
332332
}
333333

334334
public function testUpdateExpired() {
@@ -348,7 +348,7 @@ public function testUpdateExpired() {
348348
->method('updateToken')
349349
->with($this->equalTo($token));
350350

351-
$this->assertSame([], $this->controller->update($tokenId, ['filesystem' => true], 'App password'));
351+
$this->assertSame([], $this->controller->update($tokenId, [IToken::SCOPE_FILESYSTEM => true], 'App password'));
352352
}
353353

354354
public function testUpdateTokenWrongUser() {
@@ -366,7 +366,7 @@ public function testUpdateTokenWrongUser() {
366366
$this->tokenProvider->expects($this->never())
367367
->method('updateToken');
368368

369-
$response = $this->controller->update($tokenId, ['filesystem' => true], 'App password');
369+
$response = $this->controller->update($tokenId, [IToken::SCOPE_FILESYSTEM => true], 'App password');
370370
$this->assertSame([], $response->getData());
371371
$this->assertSame(\OCP\AppFramework\Http::STATUS_NOT_FOUND, $response->getStatus());
372372
}
@@ -380,7 +380,7 @@ public function testUpdateTokenNonExisting() {
380380
$this->tokenProvider->expects($this->never())
381381
->method('updateToken');
382382

383-
$response = $this->controller->update(42, ['filesystem' => true], 'App password');
383+
$response = $this->controller->update(42, [IToken::SCOPE_FILESYSTEM => true], 'App password');
384384
$this->assertSame([], $response->getData());
385385
$this->assertSame(\OCP\AppFramework\Http::STATUS_NOT_FOUND, $response->getStatus());
386386
}

‎apps/settings/tests/Settings/Personal/Security/AuthtokensTest.php

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use OCA\Settings\Settings\Personal\Security\Authtokens;
1414
use OCP\AppFramework\Http\TemplateResponse;
1515
use OCP\AppFramework\Services\IInitialState;
16+
use OCP\Authentication\Token\IToken;
1617
use OCP\ISession;
1718
use OCP\IUserSession;
1819
use PHPUnit\Framework\MockObject\MockObject;
@@ -91,7 +92,7 @@ public function testGetForm() {
9192
'type' => 0,
9293
'canDelete' => false,
9394
'current' => true,
94-
'scope' => ['filesystem' => true],
95+
'scope' => [IToken::SCOPE_FILESYSTEM => true],
9596
'canRename' => false,
9697
],
9798
[
@@ -100,7 +101,7 @@ public function testGetForm() {
100101
'lastActivity' => 0,
101102
'type' => 0,
102103
'canDelete' => true,
103-
'scope' => ['filesystem' => true],
104+
'scope' => [IToken::SCOPE_FILESYSTEM => true],
104105
'canRename' => true,
105106
],
106107
]

‎core/Controller/OCJSController.php

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

88
use bantu\IniGetWrapper\IniGetWrapper;
9+
use OC\Authentication\Token\IProvider;
910
use OC\CapabilitiesManager;
1011
use OC\Template\JSConfigHelper;
1112
use OCP\App\IAppManager;
@@ -42,6 +43,7 @@ public function __construct(
4243
IURLGenerator $urlGenerator,
4344
CapabilitiesManager $capabilitiesManager,
4445
IInitialStateService $initialStateService,
46+
IProvider $tokenProvider,
4547
) {
4648
parent::__construct($appName, $request);
4749

@@ -56,7 +58,8 @@ public function __construct(
5658
$iniWrapper,
5759
$urlGenerator,
5860
$capabilitiesManager,
59-
$initialStateService
61+
$initialStateService,
62+
$tokenProvider
6063
);
6164
}
6265

‎lib/private/AppFramework/DependencyInjection/DIContainer.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,8 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta
249249
$c->get(IControllerMethodReflector::class),
250250
$c->get(ISession::class),
251251
$c->get(IUserSession::class),
252-
$c->get(ITimeFactory::class)
252+
$c->get(ITimeFactory::class),
253+
$c->get(\OC\Authentication\Token\IProvider::class),
253254
)
254255
);
255256
$dispatcher->registerMiddleware(

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

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,18 @@
77

88
use OC\AppFramework\Middleware\Security\Exceptions\NotConfirmedException;
99
use OC\AppFramework\Utility\ControllerMethodReflector;
10+
use OC\Authentication\Token\IProvider;
1011
use OCP\AppFramework\Controller;
1112
use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired;
1213
use OCP\AppFramework\Middleware;
1314
use OCP\AppFramework\Utility\ITimeFactory;
15+
use OCP\Authentication\Exceptions\ExpiredTokenException;
16+
use OCP\Authentication\Exceptions\InvalidTokenException;
17+
use OCP\Authentication\Exceptions\WipeTokenException;
18+
use OCP\Authentication\Token\IToken;
1419
use OCP\ISession;
1520
use OCP\IUserSession;
21+
use OCP\Session\Exceptions\SessionNotAvailableException;
1622
use OCP\User\Backend\IPasswordConfirmationBackend;
1723
use ReflectionMethod;
1824

@@ -27,6 +33,7 @@ class PasswordConfirmationMiddleware extends Middleware {
2733
private $timeFactory;
2834
/** @var array */
2935
private $excludedUserBackEnds = ['user_saml' => true, 'user_globalsiteselector' => true];
36+
private IProvider $tokenProvider;
3037

3138
/**
3239
* PasswordConfirmationMiddleware constructor.
@@ -39,11 +46,14 @@ class PasswordConfirmationMiddleware extends Middleware {
3946
public function __construct(ControllerMethodReflector $reflector,
4047
ISession $session,
4148
IUserSession $userSession,
42-
ITimeFactory $timeFactory) {
49+
ITimeFactory $timeFactory,
50+
IProvider $tokenProvider,
51+
) {
4352
$this->reflector = $reflector;
4453
$this->session = $session;
4554
$this->userSession = $userSession;
4655
$this->timeFactory = $timeFactory;
56+
$this->tokenProvider = $tokenProvider;
4757
}
4858

4959
/**
@@ -68,8 +78,21 @@ public function beforeController($controller, $methodName) {
6878
$backendClassName = $user->getBackendClassName();
6979
}
7080

81+
try {
82+
$sessionId = $this->session->getId();
83+
$token = $this->tokenProvider->getToken($sessionId);
84+
} catch (SessionNotAvailableException|InvalidTokenException|WipeTokenException|ExpiredTokenException) {
85+
// States we do not deal with here.
86+
return;
87+
}
88+
$scope = $token->getScopeAsArray();
89+
if (isset($scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION]) && $scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION] === true) {
90+
// Users logging in from SSO backends cannot confirm their password by design
91+
return;
92+
}
93+
7194
$lastConfirm = (int) $this->session->get('last-password-confirm');
72-
// we can't check the password against a SAML backend, so skip password confirmation in this case
95+
// TODO: confirm excludedUserBackEnds can go away and remove it
7396
if (!isset($this->excludedUserBackEnds[$backendClassName]) && $lastConfirm < ($this->timeFactory->getTime() - (30 * 60 + 15))) { // allow 15 seconds delay
7497
throw new NotConfirmedException();
7598
}

‎lib/private/Authentication/Token/PublicKeyToken.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
namespace OC\Authentication\Token;
1010

1111
use OCP\AppFramework\Db\Entity;
12+
use OCP\Authentication\Token\IToken;
1213

1314
/**
1415
* @method void setId(int $id)
@@ -162,7 +163,7 @@ public function getScopeAsArray(): array {
162163
$scope = json_decode($this->getScope(), true);
163164
if (!$scope) {
164165
return [
165-
'filesystem' => true
166+
IToken::SCOPE_FILESYSTEM => true
166167
];
167168
}
168169
return $scope;

‎lib/private/Authentication/Token/PublicKeyTokenProvider.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,7 @@ public function renewSessionToken(string $oldSessionId, string $sessionId): OCPI
243243
OCPIToken::TEMPORARY_TOKEN,
244244
$token->getRemember()
245245
);
246+
$newToken->setScope($token->getScopeAsArray());
246247
$this->cacheToken($newToken);
247248

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

‎lib/private/Lockdown/LockdownManager.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
*/
66
namespace OC\Lockdown;
77

8-
use OC\Authentication\Token\IToken;
8+
use OCP\Authentication\Token\IToken;
99
use OCP\ISession;
1010
use OCP\Lockdown\ILockdownManager;
1111

@@ -60,6 +60,6 @@ public function setToken(IToken $token) {
6060

6161
public function canAccessFilesystem() {
6262
$scope = $this->getScopeAsArray();
63-
return !$scope || $scope['filesystem'];
63+
return !$scope || $scope[IToken::SCOPE_FILESYSTEM];
6464
}
6565
}

‎lib/private/Template/JSConfigHelper.php

Lines changed: 38 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,15 @@
88
namespace OC\Template;
99

1010
use bantu\IniGetWrapper\IniGetWrapper;
11+
use OC\Authentication\Token\IProvider;
1112
use OC\CapabilitiesManager;
1213
use OC\Share\Share;
1314
use OCP\App\AppPathNotFoundException;
1415
use OCP\App\IAppManager;
16+
use OCP\Authentication\Exceptions\ExpiredTokenException;
17+
use OCP\Authentication\Exceptions\InvalidTokenException;
18+
use OCP\Authentication\Exceptions\WipeTokenException;
19+
use OCP\Authentication\Token\IToken;
1520
use OCP\Constants;
1621
use OCP\Defaults;
1722
use OCP\Files\FileInfo;
@@ -23,48 +28,30 @@
2328
use OCP\ISession;
2429
use OCP\IURLGenerator;
2530
use OCP\IUser;
31+
use OCP\Session\Exceptions\SessionNotAvailableException;
2632
use OCP\Share\IManager as IShareManager;
2733
use OCP\User\Backend\IPasswordConfirmationBackend;
2834
use OCP\Util;
2935

3036
class JSConfigHelper {
31-
protected IL10N $l;
32-
protected Defaults $defaults;
33-
protected IAppManager $appManager;
34-
protected ISession $session;
35-
protected ?IUser $currentUser;
36-
protected IConfig $config;
37-
protected IGroupManager $groupManager;
38-
protected IniGetWrapper $iniWrapper;
39-
protected IURLGenerator $urlGenerator;
40-
protected CapabilitiesManager $capabilitiesManager;
41-
protected IInitialStateService $initialStateService;
4237

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

46-
public function __construct(IL10N $l,
47-
Defaults $defaults,
48-
IAppManager $appManager,
49-
ISession $session,
50-
?IUser $currentUser,
51-
IConfig $config,
52-
IGroupManager $groupManager,
53-
IniGetWrapper $iniWrapper,
54-
IURLGenerator $urlGenerator,
55-
CapabilitiesManager $capabilitiesManager,
56-
IInitialStateService $initialStateService) {
57-
$this->l = $l;
58-
$this->defaults = $defaults;
59-
$this->appManager = $appManager;
60-
$this->session = $session;
61-
$this->currentUser = $currentUser;
62-
$this->config = $config;
63-
$this->groupManager = $groupManager;
64-
$this->iniWrapper = $iniWrapper;
65-
$this->urlGenerator = $urlGenerator;
66-
$this->capabilitiesManager = $capabilitiesManager;
67-
$this->initialStateService = $initialStateService;
41+
public function __construct(
42+
protected IL10N $l,
43+
protected Defaults $defaults,
44+
protected IAppManager $appManager,
45+
protected ISession $session,
46+
protected ?IUser $currentUser,
47+
protected IConfig $config,
48+
protected IGroupManager $groupManager,
49+
protected IniGetWrapper $iniWrapper,
50+
protected IURLGenerator $urlGenerator,
51+
protected CapabilitiesManager $capabilitiesManager,
52+
protected IInitialStateService $initialStateService,
53+
protected IProvider $tokenProvider,
54+
) {
6855
}
6956

7057
public function getConfig(): string {
@@ -130,9 +117,13 @@ public function getConfig(): string {
130117
}
131118

132119
if ($this->currentUser instanceof IUser) {
133-
$lastConfirmTimestamp = $this->session->get('last-password-confirm');
134-
if (!is_int($lastConfirmTimestamp)) {
135-
$lastConfirmTimestamp = 0;
120+
if ($this->canUserValidatePassword()) {
121+
$lastConfirmTimestamp = $this->session->get('last-password-confirm');
122+
if (!is_int($lastConfirmTimestamp)) {
123+
$lastConfirmTimestamp = 0;
124+
}
125+
} else {
126+
$lastConfirmTimestamp = PHP_INT_MAX;
136127
}
137128
} else {
138129
$lastConfirmTimestamp = 0;
@@ -287,4 +278,15 @@ public function getConfig(): string {
287278

288279
return $result;
289280
}
281+
282+
protected function canUserValidatePassword(): bool {
283+
try {
284+
$token = $this->tokenProvider->getToken($this->session->getId());
285+
} catch (ExpiredTokenException|WipeTokenException|InvalidTokenException|SessionNotAvailableException) {
286+
// actually we do not know, so we fall back to this statement
287+
return true;
288+
}
289+
$scope = $token->getScopeAsArray();
290+
return !isset($scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION]) || $scope[IToken::SCOPE_SKIP_PASSWORD_VALIDATION] === false;
291+
}
290292
}

‎lib/private/TemplateLayout.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
namespace OC;
99

1010
use bantu\IniGetWrapper\IniGetWrapper;
11+
use OC\Authentication\Token\IProvider;
1112
use OC\Search\SearchQuery;
1213
use OC\Template\CSSResourceLocator;
1314
use OC\Template\JSConfigHelper;
@@ -225,7 +226,8 @@ public function __construct($renderAs, $appId = '') {
225226
\OC::$server->get(IniGetWrapper::class),
226227
\OC::$server->getURLGenerator(),
227228
\OC::$server->get(CapabilitiesManager::class),
228-
\OCP\Server::get(IInitialStateService::class)
229+
\OCP\Server::get(IInitialStateService::class),
230+
\OCP\Server::get(IProvider::class),
229231
);
230232
$config = $jsConfigHelper->getConfig();
231233
if (\OC::$server->getContentSecurityPolicyNonceManager()->browserSupportsCspV3()) {

0 commit comments

Comments
 (0)