Avoid updating the same oc_authtoken row twice

[juliusknorr]

• docs: Update token_auth_activity_update default value to match implementation
• fix: Always set last activity if we update the row of an authtoken anyways

Summary

Checking out query frequency on an instance I noticed that the following two queries roughly are happening quite frequently and checking the code I noticed that it might happen that we issue two update queries for the same row in a request.

UPDATE `oc_authtoken` SET `last_check` = ? WHERE `id` = ?
UPDATE `oc_authtoken` SET `last_activity` = ? WHERE ( `id` = ? ) AND ( `last_activity` < ? )

If we update the last_checked value of a row anyways I think we could directly also set the last activity value to avoid double updates of that row.

This is triggered from

server/lib/private/User/Session.php

Lines 818 to 829 in ec5133b

	if (!$this->checkTokenCredentials($dbToken, $token)) {
	$this->logger->warning('Session token credentials are invalid', [
	'app' => 'core',
	'user' => $user,
	]);
	return false;
	}
	// Update token scope
	$this->lockdownManager->setToken($dbToken);
	$this->tokenProvider->updateTokenActivity($dbToken);

where checkTokenCredentials might update the last_check value and updateTokenActivity could then perform a second update just for the activity.

I could verify this by manually update oc_authtoken set last_check = 0, last_activity = 0; to simulate older timestampts and stepping through the next request with a debugger

[joshtrichards]

Do you foresee any downsides here since we'd be bypassing the updateActivity() tolerance optimizations?

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

Lines 203 to 231 in acf8ea1

	/**
	* Update the last activity timestamp
	*
	* In highly concurrent setups it can happen that two parallel processes
	* trigger the update at (nearly) the same time. In that special case it's
	* not necessary to hit the database with two actual updates. Therefore the
	* target last activity is included in the WHERE clause with a few seconds
	* of tolerance.
	*
	* Example:
	* - process 1 (P1) reads the token at timestamp 1500
	* - process 1 (P2) reads the token at timestamp 1501
	* - activity update interval is 100
	*
	* This means
	*
	* - P1 will see a last_activity smaller than the current time and update
	* the token row
	* - If P2 reads after P1 had written, it will see 1600 as last activity
	* and the comparison on last_activity won't be truthy. This means no rows
	* need to be updated a second time
	* - If P2 reads before P1 had written, it will see 1501 as last activity,
	* but the comparison on last_activity will still not be truthy and the
	* token row is not updated a second time
	*
	* @param IToken $token
	* @param int $now
	*/
	public function updateActivity(IToken $token, int $now): void {

[juliusknorr]

I don't see any additional risk there. The row is updated anways when this is being called and I also didn't see any hints on unnecessary further updates on query statistics of the high traffic instance where this finding originated from.

[juliusknorr]

Waiting with the merge since @ChristophWurst wanted to have another close look

[ChristophWurst]

/backport to stable29

[ChristophWurst]

/backport to stable28

[ChristophWurst]

/backport to stable27

[ChristophWurst]

/backport to stable26

[ChristophWurst]

I've realized a logical issue where we now update the last_activity for tokens that are not active. E.g. \OC\Authentication\Token\PublicKeyTokenProvider::rotate, \OC\Authentication\Token\PublicKeyTokenProvider::setPassword and \OCA\Settings\Controller\AuthSettingsController::update update tokens of other clients/sessions.

Especially with the rotation and the password update we will see tokens/clients as active even if they have not connected.

[ChristophWurst]

I've realized a logical issue where we now update the last_activity for tokens that are not active

Addressed with #45411