Skip to content

Commit ddade72

Browse files
blizzzblizzz
committed
fix(Session): restore session data after cookie login
when a browser is closed and the session lost, a new session might be started with the help of the remember me cookie. For we keep some data in the session, like the IdP identifier, and some SAML data needed for SLO, those were lost in this process. This made especially logout behaviour not acting as expected. Now we keep the required data also in the database and can restore it on a cookie login event. Signed-off-by: Arthur Schiwon <[email protected]>
1 parent 3448746 commit ddade72

File tree

11 files changed

+353
-17
lines changed

11 files changed

+353
-17
lines changed

‎appinfo/info.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ The following providers are supported and tested at the moment:
2020
* Any other provider that authenticates using the environment variable
2121
2222
While theoretically any other authentication provider implementing either one of those standards is compatible, we like to note that they are not part of any internal test matrix.]]></description>
23-
<version>7.1.1</version>
23+
<version>7.1.2-beta.1</version>
2424
<licence>agpl</licence>
2525
<author>Lukas Reschke</author>
2626
<namespace>User_SAML</namespace>

‎lib/AppInfo/Application.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use OCA\User_SAML\DavPlugin;
1717
use OCA\User_SAML\GroupBackend;
1818
use OCA\User_SAML\GroupManager;
19+
use OCA\User_SAML\Listener\CookieLoginEventListener;
1920
use OCA\User_SAML\Listener\LoadAdditionalScriptsListener;
2021
use OCA\User_SAML\Listener\SabrePluginEventListener;
2122
use OCA\User_SAML\Middleware\OnlyLoggedInMiddleware;
@@ -38,6 +39,8 @@
3839
use OCP\IUserSession;
3940
use OCP\L10N\IFactory;
4041
use OCP\Server;
42+
use OCP\User\Events\BeforeUserLoggedInWithCookieEvent;
43+
use OCP\User\Events\UserLoggedInWithCookieEvent;
4144
use Psr\Container\ContainerInterface;
4245
use Psr\Log\LoggerInterface;
4346
use Throwable;
@@ -53,6 +56,8 @@ public function register(IRegistrationContext $context): void {
5356
$context->registerMiddleware(OnlyLoggedInMiddleware::class);
5457
$context->registerEventListener(BeforeTemplateRenderedEvent::class, LoadAdditionalScriptsListener::class);
5558
$context->registerEventListener(SabrePluginAddEvent::class, SabrePluginEventListener::class);
59+
$context->registerEventListener(BeforeUserLoggedInWithCookieEvent::class, CookieLoginEventListener::class);
60+
$context->registerEventListener(UserLoggedInWithCookieEvent::class, CookieLoginEventListener::class);
5661
$context->registerService(DavPlugin::class, fn (ContainerInterface $c) => new DavPlugin(
5762
$c->get(ISession::class),
5863
$c->get(IConfig::class),

‎lib/Controller/SAMLController.php‎

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use OCA\User_SAML\Exceptions\UserFilterViolationException;
1818
use OCA\User_SAML\Helper\TXmlHelper;
1919
use OCA\User_SAML\SAMLSettings;
20+
use OCA\User_SAML\Service\SessionService;
2021
use OCA\User_SAML\UserBackend;
2122
use OCA\User_SAML\UserData;
2223
use OCA\User_SAML\UserResolver;
@@ -57,6 +58,7 @@ public function __construct(
5758
private UserData $userData,
5859
private ICrypto $crypto,
5960
private ITrustedDomainHelper $trustedDomainHelper,
61+
private SessionService $sessionService,
6062
) {
6163
parent::__construct($appName, $request);
6264
}
@@ -333,10 +335,10 @@ public function assertionConsumerService(): Http\RedirectResponse {
333335
}
334336

335337
$AuthNRequestID = $data['AuthNRequestID'];
336-
$idp = $data['Idp'];
338+
$idp = $data['Idp'] ;
337339
// need to keep the IdP config ID during session lifetime (SAMLSettings::getPrefix)
338-
$this->session->set('user_saml.Idp', $idp);
339-
if (is_null($AuthNRequestID) || $AuthNRequestID === '' || is_null($idp)) {
340+
$this->sessionService->storeIdentityProviderInSession($idp);
341+
if (is_null($AuthNRequestID) || $AuthNRequestID === '') {
340342
$this->logger->debug('Invalid auth payload', ['app' => 'user_saml']);
341343
return new Http\RedirectResponse($this->urlGenerator->getAbsoluteURL('/'));
342344
}
@@ -383,14 +385,8 @@ public function assertionConsumerService(): Http\RedirectResponse {
383385
return $response;
384386
}
385387

386-
$this->session->set('user_saml.samlUserData', $auth->getAttributes());
387-
$this->session->set('user_saml.samlNameId', $auth->getNameId());
388-
$this->session->set('user_saml.samlNameIdFormat', $auth->getNameIdFormat());
389-
$this->session->set('user_saml.samlNameIdNameQualifier', $auth->getNameIdNameQualifier());
390-
$this->session->set('user_saml.samlNameIdSPNameQualifier', $auth->getNameIdSPNameQualifier());
391-
$this->session->set('user_saml.samlSessionIndex', $auth->getSessionIndex());
392-
$this->session->set('user_saml.samlSessionExpiration', $auth->getSessionExpiration());
393-
$this->logger->debug('Session values set', ['app' => 'user_saml']);
388+
$this->sessionService->prepareSession($auth);
389+
394390
try {
395391
$user = $this->userResolver->findExistingUser($this->userBackend->getCurrentUserId());
396392
$firstLogin = $user->updateLastLoginTimestamp();
@@ -510,14 +506,14 @@ public function singleLogoutService(): Http\RedirectResponse {
510506
*/
511507
private function tryProcessSLOResponse(?int $idp): array {
512508
$idps = ($idp !== null) ? [$idp] : array_keys($this->samlSettings->getListOfIdps());
513-
foreach ($idps as $idp) {
509+
foreach ($idps as $identityProviderId) {
514510
try {
515-
$auth = new Auth($this->samlSettings->getOneLoginSettingsArray($idp));
511+
$auth = new Auth($this->samlSettings->getOneLoginSettingsArray($identityProviderId));
516512
// validator (called with processSLO()) needs an XML entity loader
517513
$targetUrl = $this->callWithXmlEntityLoader(fn (): string => $auth->processSLO(
518514
true, // do not let processSLO to delete the entire session. Let userSession->logout do the job
519515
null,
520-
$this->samlSettings->usesSloWebServerDecode($idp),
516+
$this->samlSettings->usesSloWebServerDecode($identityProviderId),
521517
null,
522518
true
523519
));

‎lib/Db/SessionData.php‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-or-later
7+
*/
8+
namespace OCA\User_SAML\Db;
9+
10+
use OCA\User_SAML\Model\SessionData as SessionDataModel;
11+
use OCP\AppFramework\Db\Entity;
12+
use OCP\DB\Types;
13+
14+
class SessionData extends Entity {
15+
/** @var string */
16+
public $id;
17+
public ?string $data = null;
18+
19+
public function __construct() {
20+
$this->addType('id', Types::STRING);
21+
$this->addType('data', Types::TEXT);
22+
}
23+
24+
public function setId(string $id): void {
25+
$this->id = $id;
26+
$this->markFieldUpdated('id');
27+
}
28+
29+
public function setData(SessionDataModel $input): void {
30+
$this->data = json_encode($input);
31+
$this->markFieldUpdated('data');
32+
}
33+
34+
public function getData(): SessionDataModel {
35+
$deserialized = json_decode($this->data, true);
36+
return SessionDataModel::fromInputArray($deserialized);
37+
}
38+
}

‎lib/Db/SessionDataMapper.php‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-or-later
7+
*/
8+
namespace OCA\User_SAML\Db;
9+
10+
use OCP\AppFramework\Db\DoesNotExistException;
11+
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
12+
use OCP\AppFramework\Db\QBMapper;
13+
use OCP\DB\Exception;
14+
use OCP\IDBConnection;
15+
16+
/** @template-extends QBMapper<SessionData> */
17+
class SessionDataMapper extends QBMapper {
18+
public const SESSION_DATA_TABLE_NAME = 'user_saml_session_data';
19+
20+
public function __construct(IDBConnection $db) {
21+
parent::__construct($db, self::SESSION_DATA_TABLE_NAME, SessionData::class);
22+
}
23+
24+
/**
25+
* @throws DoesNotExistException
26+
* @throws MultipleObjectsReturnedException
27+
* @throws Exception
28+
*/
29+
public function retrieve(string $sessionId): SessionData {
30+
$qb = $this->db->getQueryBuilder();
31+
$qb->select('*')
32+
->from(self::SESSION_DATA_TABLE_NAME)
33+
->where($qb->expr()->eq('id', $qb->createNamedParameter($sessionId)));
34+
return $this->findEntity($qb);
35+
}
36+
}

‎lib/Listener/CookieLoginEventListener.php‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-or-later
7+
*/
8+
namespace OCA\User_SAML\Listener;
9+
10+
use OCA\User_SAML\Service\SessionService;
11+
use OCP\EventDispatcher\Event;
12+
use OCP\EventDispatcher\IEventListener;
13+
use OCP\User\Events\BeforeUserLoggedInWithCookieEvent;
14+
use OCP\User\Events\UserLoggedInWithCookieEvent;
15+
16+
/** @template-implements IEventListener<BeforeUserLoggedInWithCookieEvent|UserLoggedInWithCookieEvent|Event> */
17+
class CookieLoginEventListener implements IEventListener {
18+
protected ?string $oldSessionId = null;
19+
20+
public function __construct(
21+
protected readonly SessionService $sessionService,
22+
) {
23+
}
24+
25+
public function handle(Event $event): void {
26+
if ($event instanceof BeforeUserLoggedInWithCookieEvent) {
27+
$this->prepareRestoreOfSession();
28+
return;
29+
}
30+
31+
if ($event instanceof UserLoggedInWithCookieEvent) {
32+
$this->restoreSession();
33+
return;
34+
}
35+
}
36+
37+
protected function prepareRestoreOfSession(): void {
38+
if (isset($_COOKIE['nc_session_id'])) {
39+
$this->oldSessionId = $_COOKIE['nc_session_id'];
40+
}
41+
}
42+
43+
protected function restoreSession(): void {
44+
if ($this->oldSessionId !== null) {
45+
$this->sessionService->restoreSession($this->oldSessionId);
46+
}
47+
}
48+
}

‎lib/Migration/Version7001Date20251215192613.php‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\User_SAML\Migration;
11+
12+
use Closure;
13+
use OCP\DB\ISchemaWrapper;
14+
use OCP\DB\Types;
15+
use OCP\Migration\IOutput;
16+
use OCP\Migration\SimpleMigrationStep;
17+
use Override;
18+
19+
class Version7001Date20251215192613 extends SimpleMigrationStep {
20+
public const SESSION_DATA_TABLE_NAME = 'user_saml_session_data';
21+
22+
#[Override]
23+
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
24+
/** @var ISchemaWrapper $schema */
25+
$schema = $schemaClosure();
26+
27+
if ($schema->hasTable(self::SESSION_DATA_TABLE_NAME)) {
28+
return null;
29+
}
30+
31+
$table = $schema->createTable(self::SESSION_DATA_TABLE_NAME);
32+
$table->addColumn('id', Types::STRING, [
33+
'notnull' => true,
34+
'length' => 200,
35+
]);
36+
$table->addColumn('data', Types::TEXT, [
37+
'notnull' => true,
38+
]);
39+
$table->setPrimaryKey(['id']);
40+
41+
return $schema;
42+
}
43+
44+
}

‎lib/Model/SessionData.php‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-or-later
7+
*/
8+
namespace OCA\User_SAML\Model;
9+
10+
use OCP\ISession;
11+
12+
class SessionData implements \JsonSerializable, \Stringable {
13+
public const KEY_IDENTITY_PROVIDER_ID = 'user_saml.Idp';
14+
public const KEY_SAML_NAME_ID = 'user_saml.samlNameId';
15+
public const KEY_SAML_NAME_ID_FORMAT = 'user_saml.samlNameIdFormat';
16+
public const KEY_SAML_NAME_ID_QUALIFIER = 'user_saml.samlNameIdNameQualifier';
17+
public const KEY_SAML_NAME_ID_SP_QUALIFIER = 'user_saml.samlNameIdSPNameQualifier';
18+
public const KEY_SAML_SESSION_INDEX = 'user_saml.samlSessionIndex';
19+
20+
public const SESSION_KEYS = [
21+
'KEY_IDENTITY_PROVIDER_ID' => 'user_saml.Idp',
22+
'KEY_SAML_NAME_ID' => 'user_saml.samlNameId',
23+
'KEY_SAML_NAME_ID_FORMAT' => 'user_saml.samlNameIdFormat',
24+
'KEY_SAML_NAME_ID_QUALIFIER' => 'user_saml.samlNameIdNameQualifier',
25+
'KEY_SAML_NAME_ID_SP_QUALIFIER' => 'user_saml.samlNameIdSPNameQualifier',
26+
'KEY_SAML_SESSION_INDEX' => 'user_saml.samlSessionIndex',
27+
];
28+
29+
public function __construct(
30+
protected int $identityProviderId,
31+
protected string $samlNameId,
32+
protected string $samlNameIdFormat,
33+
protected string $samlNameIdNameQualifier,
34+
protected string $samlNameIdSPNameQualifier,
35+
protected ?string $samlSessionIndex,
36+
) {
37+
if ($this->identityProviderId < 0) {
38+
throw new \InvalidArgumentException('IdentityProviderId has to be a positive integer');
39+
}
40+
}
41+
42+
public function storeInSession(ISession $session): void {
43+
foreach ($this->jsonSerialize() as $sessionKey => $value) {
44+
$session->set($sessionKey, $value);
45+
}
46+
}
47+
48+
public function jsonSerialize(): array {
49+
return [
50+
self::KEY_IDENTITY_PROVIDER_ID => $this->identityProviderId,
51+
self::KEY_SAML_NAME_ID => $this->samlNameId,
52+
self::KEY_SAML_NAME_ID_FORMAT => $this->samlNameIdFormat,
53+
self::KEY_SAML_NAME_ID_QUALIFIER => $this->samlNameIdNameQualifier,
54+
self::KEY_SAML_NAME_ID_SP_QUALIFIER => $this->samlNameIdSPNameQualifier,
55+
self::KEY_SAML_SESSION_INDEX => $this->samlSessionIndex,
56+
];
57+
}
58+
59+
public static function fromInputArray(array $rawData): self {
60+
if (!isset($rawData[self::KEY_IDENTITY_PROVIDER_ID])) {
61+
throw new \InvalidArgumentException('Expected and required Identity Provider ID is missing');
62+
}
63+
64+
return new self(
65+
$rawData[self::KEY_IDENTITY_PROVIDER_ID],
66+
$rawData[self::KEY_SAML_NAME_ID] ?? '',
67+
$rawData[self::KEY_SAML_NAME_ID_FORMAT] ?? '',
68+
$rawData[self::KEY_SAML_NAME_ID_QUALIFIER] ?? '',
69+
$rawData[self::KEY_SAML_NAME_ID_SP_QUALIFIER] ?? '',
70+
$rawData[self::KEY_SAML_SESSION_INDEX] ?? null,
71+
);
72+
}
73+
74+
public static function fromSession(ISession $session): self {
75+
$retrievedData = [];
76+
foreach (self::SESSION_KEYS as $sessionKey) {
77+
$value = $session->get($sessionKey);
78+
if ($value === null) {
79+
continue;
80+
}
81+
$retrievedData[$sessionKey] = $session->get($sessionKey);
82+
}
83+
return self::fromInputArray($retrievedData);
84+
}
85+
86+
public function __toString(): string {
87+
return \json_encode($this);
88+
}
89+
}

0 commit comments

Comments
 (0)