Handle token insert conflicts

[ChristophWurst]

Env-based SAML uses the "Apache auth" mechanism to log users in. In this
code path, we first delete all existin auth tokens from the database,
before a new one is inserted. This is problematic for concurrent
requests as they might reach the same code at the same time, hence both
trying to insert a new row wit the same token (the session ID). This
also bubbles up and disables user_saml.

As the token might still be OK (both request will insert the same data),
we can actually just check if the UIDs of the conflict row is the same
as the one we want to insert right now. In that case let's just use the
existing entry and carry on.

Fixes nextcloud/user_saml#115

[rullzer]

What is the code path taken here?
As in why would it happen?

[ChristophWurst]

https://github.com/nextcloud/user_saml/blob/42d5f01b97060c8e3f8f1f04b8e3667d3da7bf5c/appinfo/app.php#L83

server/lib/private/legacy/user.php

Line 218 in 224073e

return self::loginWithApache($backend);

server/lib/private/legacy/user.php

Line 177 in 224073e

$userSession->createSessionToken($request, $uid, $uid);

[rullzer]

maybe I am stupid. But I still don't get how this lead to the error...

[ChristophWurst]

That code calls \OC\User\Session::createSessionToken. And this is the problematic section:

server/lib/private/User/Session.php

Lines 681 to 682 in 224073e

	$this->tokenProvider->invalidateToken($sessionId);
	$this->tokenProvider->generateToken($sessionId, $uid, $loginName, $pwd, $name, IToken::TEMPORARY_TOKEN, $remember);

This means. Every request tries to create its session token for the current session. The naive strategy to prevent conflicts is to first delete (invalidate) existing tokens with the same session ID:

server/lib/private/User/Session.php

Line 681 in 224073e

$this->tokenProvider->invalidateToken($sessionId);

.

The problem here is that this section can reached by two independent requests of the same session at the same time. So they both clear the existing tokens. So far, so good. But then one is lucky and successfully inserts its token into the database. When the second process tries to do the same, it runs into a conflict.

Does this make sense?

[rullzer]

Wouldn't just fetching the token and checking then make more sense?

[ChristophWurst]

Wouldn't just fetching the token and checking then make more sense?

The same idea came into my mind. But it doesn't solve the problem. If you have two concurrent requests that run through the section of checking if an existing token exists at the same time, they will both get back no. Thus both will insert a row -> 💥 just like the existing code.

[rullzer]

Ah mmm. Right.

But then we probably should make sure the tokens are indeed the same before returning it right? (Password, loginame etc)? Just to avoid weird bugs.

[ChristophWurst]

But then we probably should make sure the tokens are indeed the same before returning it right? (Password, loginame etc)? Just to avoid weird bugs.

Yes, that is what I started at https://github.com/nextcloud/server/pull/17939/files#diff-b72615e7b726afb43f6043a7ec472487R82. Do we have to compare that many attributes? I think we still can rely on php not assigning the same session ID twice. Hence this should always be from the exact same session.

[rullzer]

Right. if that happens we have other troubles anyways
lets do this then.

[ChristophWurst]

/backport to stable17

[ChristophWurst]

/backport to stable16

[ChristophWurst]

/backport to stable15

[backportbot-nextcloud]

The backport to stable17 failed. Please do this backport manually.

[backportbot-nextcloud]

The backport to stable16 failed. Please do this backport manually.

[backportbot-nextcloud]

The backport to stable15 failed. Please do this backport manually.