fix(Session): restore session data after cookie login #1022
Conversation
blizzz
force-pushed
the
fix/noid/restore-session-state-after-cookie-login
branch
5 times, most recently
from
0e2e0fb to
c34215d
Compare
blizzz
force-pushed
the
fix/noid/restore-session-state-after-cookie-login
branch
from
c34215d to
cc1cecd
Compare
blizzz
force-pushed
the
fix/noid/restore-session-state-after-cookie-login
branch
2 times, most recently
from
9433ae4 to
33a91c5
Compare
| 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> | ||
| <version>7.1.1</version> | ||
| <version>7.1.2-beta.1</version> |
There was a problem hiding this comment.
As the DB schema changes, this actually needs a bump to 8.0.0 🤔 However. it kills backportability to lower versions (not intended yet, but you never know). Would hence go to 7.2.0 instead although it technically is not correct. Opinions?
There was a problem hiding this comment.
AFAIK there is no hard requirement on needing a major bump for db changes. Even patches can have them. Admins are not fans, but it's inevitable for some changes.
There was a problem hiding this comment.
The spec is unclear in its current form, but as it is an external system and it requires the changed schema, you can argue it falls into the "public api" scope. This is also largely discussed and acknowledged in semver/semver#90
If not bumping the major with a DB schema change is lived practice across Nextcloud (I am not sure, but your statement sounds like it), then it is acceptable to only bump the patch number.
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. Storing the session data in the database can only happen after the session token was created, i.e. when the UserLoggedInEvent was dispatched. Stored session data is deleted once the original authtoken has also disappeared. A daily check during maintenance times is scheduled. Because hashing of the session id is a private implementation detail of the current token provider, we track the id of the token as well so that any future change in hashing will not have any effect. Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
blizzz
force-pushed
the
fix/noid/restore-session-state-after-cookie-login
branch
from
33a91c5 to
cc0d7a6
Compare
| 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> | ||
| <version>7.1.1</version> | ||
| <version>7.1.2-beta.1</version> |
There was a problem hiding this comment.
AFAIK there is no hard requirement on needing a major bump for db changes. Even patches can have them. Admins are not fans, but it's inevitable for some changes.
| "psalm": "psalm", | ||
| "psalm:fix": "psalm --alter --issues=InvalidReturnType,InvalidNullableReturnType,MissingParamType,InvalidFalsableReturnType", | ||
| "psalm:update-baseline": "psalm --threads=1 --update-baseline", | ||
| "cs:fix": "@php php-cs-fixer fix", |
| public function restoreSession(string $oldSessionId): void { | ||
| try { | ||
| $sessionIdHash = $this->hashSessionId($oldSessionId); | ||
| $sessionData = $this->sessionDataMapper->retrieve($sessionIdHash); |
There was a problem hiding this comment.
Should we store the user_id together with the session data as additional hardening?
Looks like we only reach this point when username + token + session_id matches?
There was a problem hiding this comment.
Since we restore the session when the successful-cookie-login-event has happened, this check was run already by the server and we can rely on it. No necessity to repeat the check here.
The method is also doing exactly what it is named after, and the usage is correct – after a successful check, indicated by the event of the accomplished cookie login.
Follow-up of #1022. We need to use IAppConfig now Signed-off-by: Louis Chmn <louis@chmn.me>
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.
Fixes #998