-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix missing token update when another field (e.g. last checked) changed #29681
Conversation
Signed-off-by: Joas Schilling <[email protected]>
/backport to stable23 |
/backport to stable22 |
Signed-off-by: Joas Schilling <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change make sense
Signed-off-by: Joas Schilling <[email protected]>
@@ -128,6 +128,8 @@ public function updateTokenActivity(IToken $token) { | |||
// Update token only once per minute | |||
$token->setLastActivity($now); | |||
$this->mapper->update($token); | |||
} elseif (!empty($token->getUpdatedFields())) { | |||
$this->mapper->update($token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while this fixes the issue it's weird that we do this regular update in a updateTokenActivity
method, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only alternate approach is to either bring the double update back, or to move the acivity time calculation to the session and make the "which update to use" decision there:
server/lib/private/User/Session.php
Lines 809 to 811 in a143337
$this->tokenProvider->updateTokenActivity($dbToken); | |
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the double update is the subjectively cleanest as changes are written right where they are made
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also fine by me
Replaced by #29682 |
Conflicting improvements from:
Fix #29678