Skip to content

Commit 7217c0c

Browse files
icewind1991icewind1991
committed
merge last_activity and last_check updates
the debounce for updating last_activity is changed so it always updates if another field of the token has been updated, this ensures that last_check if updated even if last_activity is still in the debounce period. Signed-off-by: Robin Appelman <robin@icewind.nl>
1 parent 9104028 commit 7217c0c

File tree

6 files changed

+108
-33
lines changed

6 files changed

+108
-33
lines changed

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

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ public function hasExpiredTokens(string $uid): bool {
192192
}
193193

194194
/**
195-
* Update the last activity timestamp
195+
* Update the last activity timestamp and save all saved fields
196196
*
197197
* In highly concurrent setups it can happen that two parallel processes
198198
* trigger the update at (nearly) the same time. In that special case it's
@@ -202,7 +202,7 @@ public function hasExpiredTokens(string $uid): bool {
202202
*
203203
* Example:
204204
* - process 1 (P1) reads the token at timestamp 1500
205-
* - process 1 (P2) reads the token at timestamp 1501
205+
* - process 2 (P2) reads the token at timestamp 1501
206206
* - activity update interval is 100
207207
*
208208
* This means
@@ -216,17 +216,21 @@ public function hasExpiredTokens(string $uid): bool {
216216
* but the comparison on last_activity will still not be truthy and the
217217
* token row is not updated a second time
218218
*
219-
* @param IToken $token
219+
* @param PublicKeyToken $token
220220
* @param int $now
221221
*/
222-
public function updateActivity(IToken $token, int $now): void {
223-
$qb = $this->db->getQueryBuilder();
224-
$update = $qb->update($this->getTableName())
225-
->set('last_activity', $qb->createNamedParameter($now, IQueryBuilder::PARAM_INT))
226-
->where(
227-
$qb->expr()->eq('id', $qb->createNamedParameter($token->getId(), IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT),
228-
$qb->expr()->lt('last_activity', $qb->createNamedParameter($now - 15, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT)
229-
);
222+
public function updateActivity(PublicKeyToken $token, int $now): void {
223+
$token->setLastActivity($now);
224+
$update = $this->createUpdateQuery($token);
225+
226+
$updatedFields = $token->getUpdatedFields();
227+
unset($updatedFields['lastActivity']);
228+
229+
// if no other fields are updated, we add the extra filter to prevent duplicate updates
230+
if (count($updatedFields) === 0) {
231+
$update->andWhere($update->expr()->lt('last_activity', $update->createNamedParameter($now - 15, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT));
232+
}
233+
230234
$update->executeStatement();
231235
}
232236
}

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -227,10 +227,12 @@ public function updateTokenActivity(IToken $token) {
227227
$activityInterval = $this->config->getSystemValueInt('token_auth_activity_update', 60);
228228
$activityInterval = min(max($activityInterval, 0), 300);
229229

230+
$updatedFields = $token->getUpdatedFields();
231+
unset($updatedFields['lastActivity']);
232+
230233
/** @var PublicKeyToken $token */
231234
$now = $this->time->getTime();
232-
if ($token->getLastActivity() < ($now - $activityInterval)) {
233-
$token->setLastActivity($now);
235+
if ($token->getLastActivity() < ($now - $activityInterval) || count($updatedFields)) {
234236
$this->mapper->updateActivity($token, $now);
235237
}
236238
}

‎lib/private/User/Session.php‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -738,7 +738,6 @@ private function checkTokenCredentials(IToken $dbToken, $token) {
738738
}
739739

740740
$dbToken->setLastCheck($now);
741-
$this->tokenProvider->updateToken($dbToken);
742741
return true;
743742
}
744743

@@ -756,7 +755,6 @@ private function checkTokenCredentials(IToken $dbToken, $token) {
756755
}
757756

758757
$dbToken->setLastCheck($now);
759-
$this->tokenProvider->updateToken($dbToken);
760758
return true;
761759
}
762760

‎lib/public/AppFramework/Db/QBMapper.php‎

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -170,23 +170,8 @@ public function insertOrUpdate(Entity $entity): Entity {
170170
}
171171
}
172172

173-
/**
174-
* Updates an entry in the db from an entity
175-
*
176-
* @param Entity $entity the entity that should be created
177-
* @psalm-param T $entity the entity that should be created
178-
* @return Entity the saved entity with the set id
179-
* @psalm-return T the saved entity with the set id
180-
* @throws Exception
181-
* @throws \InvalidArgumentException if entity has no id
182-
* @since 14.0.0
183-
*/
184-
public function update(Entity $entity): Entity {
185-
// if entity wasn't changed it makes no sense to run a db query
173+
protected function createUpdateQuery(Entity $entity) {
186174
$properties = $entity->getUpdatedFields();
187-
if (\count($properties) === 0) {
188-
return $entity;
189-
}
190175

191176
// entity needs an id
192177
$id = $entity->getId();
@@ -218,6 +203,29 @@ public function update(Entity $entity): Entity {
218203
$qb->where(
219204
$qb->expr()->eq('id', $qb->createNamedParameter($id, $idType))
220205
);
206+
207+
return $qb;
208+
}
209+
210+
/**
211+
* Updates an entry in the db from an entity
212+
*
213+
* @param Entity $entity the entity that should be created
214+
* @psalm-param T $entity the entity that should be created
215+
* @return Entity the saved entity with the set id
216+
* @psalm-return T the saved entity with the set id
217+
* @throws Exception
218+
* @throws \InvalidArgumentException if entity has no id
219+
* @since 14.0.0
220+
*/
221+
public function update(Entity $entity): Entity {
222+
// if entity wasn't changed it makes no sense to run a db query
223+
$properties = $entity->getUpdatedFields();
224+
if (\count($properties) === 0) {
225+
return $entity;
226+
}
227+
228+
$qb = $this->createUpdateQuery($entity);
221229
$qb->executeStatement();
222230

223231
return $entity;

‎tests/lib/Authentication/Token/PublicKeyTokenMapperTest.php‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,4 +266,53 @@ public function testHasExpiredTokens() {
266266
$this->assertFalse($this->mapper->hasExpiredTokens('user1'));
267267
$this->assertTrue($this->mapper->hasExpiredTokens('user3'));
268268
}
269+
270+
public function testUpdateTokenActivity() {
271+
$token = '6d9a290d239d09f2cc33a03cc54cccd46f7dc71630dcc27d39214824bd3e093f1feb4e2b55eb159d204caa15dee9556c202a5aa0b9d67806c3f4ec2cde11af67';
272+
$dbToken = $this->mapper->getToken($token);
273+
274+
$this->assertEquals($dbToken->getLastActivity(), $this->time - 120);
275+
$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
276+
277+
$this->mapper->updateActivity($dbToken, $this->time);
278+
279+
$updatedDbToken = $this->mapper->getToken($token);
280+
281+
$this->assertEquals($this->time, $updatedDbToken->getLastActivity());
282+
$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
283+
$this->assertEquals($this->time, $dbToken->getLastActivity());
284+
}
285+
286+
public function testUpdateTokenActivityDebounce() {
287+
$token = '6d9a290d239d09f2cc33a03cc54cccd46f7dc71630dcc27d39214824bd3e093f1feb4e2b55eb159d204caa15dee9556c202a5aa0b9d67806c3f4ec2cde11af67';
288+
$dbToken = $this->mapper->getToken($token);
289+
290+
$this->assertEquals($dbToken->getLastActivity(), $this->time - 120);
291+
$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
292+
293+
$this->mapper->updateActivity($dbToken, $this->time - 110);
294+
295+
$updatedDbToken = $this->mapper->getToken($token);
296+
297+
$this->assertEquals($this->time - 120, $updatedDbToken->getLastActivity());
298+
$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
299+
$this->assertEquals($this->time - 110, $dbToken->getLastActivity());
300+
}
301+
302+
public function testUpdateTokenActivityDebounceUpdate() {
303+
$token = '6d9a290d239d09f2cc33a03cc54cccd46f7dc71630dcc27d39214824bd3e093f1feb4e2b55eb159d204caa15dee9556c202a5aa0b9d67806c3f4ec2cde11af67';
304+
$dbToken = $this->mapper->getToken($token);
305+
306+
$this->assertEquals($this->time - 120, $dbToken->getLastActivity());
307+
$this->assertEquals($this->time - 60 * 10, $dbToken->getLastCheck());
308+
309+
$dbToken->setLastCheck($this->time - 100);
310+
$this->mapper->updateActivity($dbToken, $this->time - 110);
311+
312+
$updatedDbToken = $this->mapper->getToken($token);
313+
314+
$this->assertEquals($this->time - 110, $updatedDbToken->getLastActivity());
315+
$this->assertEquals($this->time - 100, $dbToken->getLastCheck());
316+
$this->assertEquals($this->time - 110, $dbToken->getLastActivity());
317+
}
269318
}

‎tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -177,8 +177,6 @@ public function testUpdateToken() {
177177
]);
178178

179179
$this->tokenProvider->updateTokenActivity($tk);
180-
181-
$this->assertEquals($this->time, $tk->getLastActivity());
182180
}
183181

184182
public function testUpdateTokenDebounce() {
@@ -196,6 +194,22 @@ public function testUpdateTokenDebounce() {
196194
$this->tokenProvider->updateTokenActivity($tk);
197195
}
198196

197+
public function testUpdateTokenDebounceWithUpdatedFields() {
198+
$tk = new PublicKeyToken();
199+
$this->config->method('getSystemValueInt')
200+
->willReturnCallback(function ($value, $default) {
201+
return $default;
202+
});
203+
$tk->setLastActivity($this->time - 30);
204+
$tk->setLastCheck($this->time - 30);
205+
206+
$this->mapper->expects($this->once())
207+
->method('updateActivity')
208+
->with($tk, $this->time);
209+
210+
$this->tokenProvider->updateTokenActivity($tk);
211+
}
212+
199213
public function testGetTokenByUser() {
200214
$this->mapper->expects($this->once())
201215
->method('getTokenByUser')

0 commit comments

Comments
 (0)