fix(session): Replace remember-me tokens in transaction #40628
Conversation
ChristophWurst
added
bug
3. to review
| */ | ||
| $newToken = $this->atomic(function() use ($uid, $currentToken): ?string { | ||
| // get stored tokens | ||
| $tokens = $this->config->getUserKeys($uid, 'login_token'); |
There was a problem hiding this comment.
food for thought: we might be able to skip the SELECT and go for the DELETE directly
There was a problem hiding this comment.
Are you doing the select so you have that explict? As to ensure no database funny buisness goes on when you do a delete+insert in the transaction?
There was a problem hiding this comment.
the DELETE and INSERT should not trigger a conflict, even in concurrent situations, because the unique contraint is on appid and configkey. configkey is the new, random token.
only in theory there might be two processes with the same random token ;-)
ChristophWurst
added
2. developing
lib/private/User/Session.php
Outdated
| }, $this->connection); | ||
|
|
||
| // Token verification or replacement failed. Session can't be revived. | ||
| if ($newToken === null) { |
There was a problem hiding this comment.
so if the transaction aborts it just defaults to null? Or is there an exception that needs handling somewhere?
There was a problem hiding this comment.
the transaction commits. only unhandled exceptions inside the closure cause a rollback.
null means either SELECT found no results or DELETE did not hit any rows
| * @param string $key the key under which the value is being stored | ||
| */ | ||
| public function deleteUserValue($userId, $appName, $key) { | ||
| public function deleteUserValue($userId, $appName, $key): bool { |
ChristophWurst
added
1. to develop
skjnldsv
added
2. developing
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
ChristophWurst
force-pushed
the
fix/session/transactional-remember-me-renewal
branch
from
455aee3
to
908c381
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 'uid' => $uid, | ||
| 'user' => $uid, | ||
| ]); | ||
| return false; |
Check failure
Code scanning / Psalm
FalsableReturnStatement Error
Summary
Running
delete from oc_preferences where …in two READ COMMITTED transactions makes the database serialize the operation and allow detection of a second DELETE that happened concurrently.This is not a real fix but it can help us get a better insight into the problem.
TODO
Checklist