From 0299ea0a965b783a5b4939bcd35ffa8ac0f1a8e8 Mon Sep 17 00:00:00 2001
From: Christoph Wurst <christoph@winzerhof-wurst.at>
Date: Thu, 14 Nov 2019 12:14:26 +0100
Subject: [PATCH] Handle token insert conflicts

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.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
---
 lib/private/Authentication/Token/Manager.php  | 33 ++++++++----
 .../lib/Authentication/Token/ManagerTest.php  | 54 +++++++++++++++----
 2 files changed, 68 insertions(+), 19 deletions(-)

diff --git a/lib/private/Authentication/Token/Manager.php b/lib/private/Authentication/Token/Manager.php
index ea94efce54..0642d4426c 100644
--- a/lib/private/Authentication/Token/Manager.php
+++ b/lib/private/Authentication/Token/Manager.php
@@ -23,6 +23,7 @@ declare(strict_types=1);
 
 namespace OC\Authentication\Token;
 
+use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
 use OC\Authentication\Exceptions\ExpiredTokenException;
 use OC\Authentication\Exceptions\InvalidTokenException;
 use OC\Authentication\Exceptions\PasswordlessTokenException;
@@ -60,15 +61,29 @@ class Manager implements IProvider {
 								  string $name,
 								  int $type = IToken::TEMPORARY_TOKEN,
 								  int $remember = IToken::DO_NOT_REMEMBER): IToken {
-		return $this->publicKeyTokenProvider->generateToken(
-			$token,
-			$uid,
-			$loginName,
-			$password,
-			$name,
-			$type,
-			$remember
-		);
+		try {
+			return $this->publicKeyTokenProvider->generateToken(
+				$token,
+				$uid,
+				$loginName,
+				$password,
+				$name,
+				$type,
+				$remember
+			);
+		} catch (UniqueConstraintViolationException $e) {
+			// It's rare, but if two requests of the same session (e.g. env-based SAML)
+			// try to create the session token they might end up here at the same time
+			// because we use the session ID as token and the db token is created anew
+			// with every request.
+			//
+			// If the UIDs match, then this should be fine.
+			$existing = $this->getToken($token);
+			if ($existing->getUID() !== $uid) {
+				throw new \Exception('Token conflict handled, but UIDs do not match. This should not happen', 0, $e);
+			}
+			return $existing;
+		}
 	}
 
 	/**
diff --git a/tests/lib/Authentication/Token/ManagerTest.php b/tests/lib/Authentication/Token/ManagerTest.php
index 48876cade6..1f3fda1b15 100644
--- a/tests/lib/Authentication/Token/ManagerTest.php
+++ b/tests/lib/Authentication/Token/ManagerTest.php
@@ -1,4 +1,5 @@
-<?php
+<?php declare(strict_types=1);
+
 /**
  * @copyright Copyright (c) 2018 Roeland Jago Douma <roeland@famdouma.nl>
  *
@@ -23,6 +24,7 @@
 
 namespace Test\Authentication\Token;
 
+use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
 use OC\Authentication\Exceptions\InvalidTokenException;
 use OC\Authentication\Exceptions\PasswordlessTokenException;
 use OC\Authentication\Token\DefaultToken;
@@ -31,21 +33,15 @@ use OC\Authentication\Token\ExpiredTokenException;
 use OC\Authentication\Token\IToken;
 use OC\Authentication\Token\Manager;
 use OC\Authentication\Token\PublicKeyToken;
-use OC\Authentication\Token\PublicKeyTokenMapper;
 use OC\Authentication\Token\PublicKeyTokenProvider;
-use OCP\AppFramework\Db\DoesNotExistException;
-use OCP\AppFramework\Utility\ITimeFactory;
-use OCP\IConfig;
-use OCP\ILogger;
-use OCP\IUser;
-use OCP\Security\ICrypto;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
 
 class ManagerTest extends TestCase {
 
-	/** @var PublicKeyTokenProvider|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var PublicKeyTokenProvider|MockObject */
 	private $publicKeyTokenProvider;
-	/** @var DefaultTokenProvider|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var DefaultTokenProvider|MockObject */
 	private $defaultTokenProvider;
 	/** @var Manager */
 	private $manager;
@@ -92,6 +88,44 @@ class ManagerTest extends TestCase {
 		$this->assertSame($token, $actual);
 	}
 
+	public function testGenerateConflictingToken() {
+		/** @var MockObject|UniqueConstraintViolationException $exception */
+		$exception = $this->createMock(UniqueConstraintViolationException::class);
+		$this->defaultTokenProvider->expects($this->never())
+			->method('generateToken');
+
+		$token = new PublicKeyToken();
+		$token->setUid('uid');
+
+		$this->publicKeyTokenProvider->expects($this->once())
+			->method('generateToken')
+			->with(
+				'token',
+				'uid',
+				'loginName',
+				'password',
+				'name',
+				IToken::TEMPORARY_TOKEN,
+				IToken::REMEMBER
+			)->willThrowException($exception);
+		$this->publicKeyTokenProvider->expects($this->once())
+			->method('getToken')
+			->with('token')
+			->willReturn($token);
+
+		$actual = $this->manager->generateToken(
+			'token',
+			'uid',
+			'loginName',
+			'password',
+			'name',
+			IToken::TEMPORARY_TOKEN,
+			IToken::REMEMBER
+		);
+
+		$this->assertSame($token, $actual);
+	}
+
 	public function tokenData(): array {
 		return [
 			[new DefaultToken()],