dont update the auth token twice

[icewind1991]

The token is already updated in updateTokenActivity

Saves an update query for each request made

stacktraces for both queries on master:

cc @ChristophWurst

[LukasReschke]

is that one then required at all?

[ChristophWurst]

yes, otherwise the checks are performed on every request, see above

if ($lastCheck > ($now - 60 * 5)) {
    // Checked performed recently, nothing to do now
    return true;
}

[ChristophWurst]

Hm, $this->tokenProvider->updateTokenActivity($dbToken); updates the last_check just coincidentally. Not sure if we should rely on that magic.

[ChristophWurst]

if only the activity was updated recently, the update would be ignored then. See

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

Lines 107 to 124 in 42ef336

	/**
	* Update token activity timestamp
	*
	* @throws InvalidTokenException
	* @param IToken $token
	*/
	public function updateTokenActivity(IToken $token) {
	if (!($token instanceof DefaultToken)) {
	throw new InvalidTokenException();
	}
	/** @var DefaultToken $token */
	$now = $this->time->getTime();
	if ($token->getLastActivity() < ($now - 60)) {
	// Update token only once per minute
	$token->setLastActivity($now);
	$this->mapper->update($token);
	}
	}

[icewind1991]

since lastActivity is updated more often than lastChecked that shouldn't be a problem, it will still recheck the token every 5 min.

The only "loss" is that lastActivity can lag behind a max of 1min, but that also means that we only have 1 update query for tokens per min which should help with db load on large systems (updates/inserts being way more expensive than reads)

[rullzer]

I agree with @icewind1991 that lagging behind a bit is not that big of a deal. If it is max 1 minute. Fine by me.

But I also agree with @ChristophWurst that we should not depend on dark magic. So please make sure it is properly tested.

Will take a more in depth look tomorrow.

[MorrisJobke]

Will take a more in depth look tomorrow.

Tomorrow is over 😉

[rullzer]

Tomorrow is over 😉

True ;)

I'm still torn. I like the improvement but I feel this will break once somebody at some point moves code around. So can we have unit tests for this behaviour? Then it is 🚀 for me!

[MorrisJobke]

I'm still torn. I like the improvement but I feel this will break once somebody at some point moves code around. So can we have unit tests for this behaviour? Then it is for me!

@icewind1991 Could you add unit tests for this? Thanks

[icewind1991]

Added tests to ensure the token times are updated

[MorrisJobke]

There was 1 failure:
1) Test\User\SessionTest::testValidateSessionNoPassword
Expectation failed for method name is equal to <string:updateToken> when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

[MorrisJobke]

Tested and works 👍

[codecov-io]

Current coverage is 56.39% (diff: 100%)

Merging #1037 into master will decrease coverage by 0.01%

@@             master      #1037   diff @@
==========================================
  Files          1070       1070          
  Lines         60432      60490    +58   
  Methods        6817       6825     +8   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          34087      34111    +24   
- Misses        26345      26379    +34   
  Partials          0          0          

Diff Coverage	File Path
•••••••••• 100%	lib/private/User/Session.php

Powered by Codecov. Last update cc4b514...90db361

[rullzer]

ok test. Fine by me. LGTM then.
@ChristophWurst you are fine as well?

[ChristophWurst]

👍 I guess 🙈