From bbe41a73dd0a77c572c78e9df8a77ff7b0cb7bf2 Mon Sep 17 00:00:00 2001
From: Daniel Kesselberg <mail@danielkesselberg.de>
Date: Wed, 9 Oct 2019 17:43:54 +0200
Subject: [PATCH 1/2] Return a default user record if json is broken

It's possible that json_decode returns null. Mostly the json is broken.
AddMissingDefaultValues expects an array. Pass null will fail.

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
---
 lib/private/Accounts/AccountManager.php | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php
index f1607b1a1e..01920d487b 100644
--- a/lib/private/Accounts/AccountManager.php
+++ b/lib/private/Accounts/AccountManager.php
@@ -137,6 +137,9 @@ class AccountManager implements IAccountManager {
 		}
 
 		$userDataArray = json_decode($result[0]['data'], true);
+		if ($userDataArray === null || json_last_error() !== JSON_ERROR_NONE) {
+			return $this->buildDefaultUserRecord($user);
+		}
 
 		$userDataArray = $this->addMissingDefaultValues($userDataArray);
 

From b97d90e0c3a9064127cf203e92c5c18bde129703 Mon Sep 17 00:00:00 2001
From: Christoph Wurst <christoph@winzerhof-wurst.at>
Date: Fri, 13 Dec 2019 12:39:29 +0100
Subject: [PATCH 2/2] Log critical fallback to user default if we can't parse
 the JSON

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
---
 lib/private/Accounts/AccountManager.php    | 15 +++++++++++++--
 lib/private/Accounts/Hooks.php             |  8 ++------
 tests/lib/Accounts/AccountsManagerTest.php | 19 ++++++++++++-------
 3 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php
index 01920d487b..225d076ca3 100644
--- a/lib/private/Accounts/AccountManager.php
+++ b/lib/private/Accounts/AccountManager.php
@@ -1,4 +1,5 @@
 <?php
+
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  * @copyright Copyright (c) 2016, Björn Schießle
@@ -33,9 +34,12 @@ use OCP\Accounts\IAccount;
 use OCP\Accounts\IAccountManager;
 use OCP\BackgroundJob\IJobList;
 use OCP\IDBConnection;
+use OCP\ILogger;
 use OCP\IUser;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\EventDispatcher\GenericEvent;
+use function json_decode;
+use function json_last_error;
 
 /**
  * Class AccountManager
@@ -59,6 +63,9 @@ class AccountManager implements IAccountManager {
 	/** @var IJobList */
 	private $jobList;
 
+	/** @var ILogger */
+	private $logger;
+
 	/**
 	 * AccountManager constructor.
 	 *
@@ -68,10 +75,12 @@ class AccountManager implements IAccountManager {
 	 */
 	public function __construct(IDBConnection $connection,
 								EventDispatcherInterface $eventDispatcher,
-								IJobList $jobList) {
+								IJobList $jobList,
+								ILogger $logger) {
 		$this->connection = $connection;
 		$this->eventDispatcher = $eventDispatcher;
 		$this->jobList = $jobList;
+		$this->logger = $logger;
 	}
 
 	/**
@@ -137,7 +146,9 @@ class AccountManager implements IAccountManager {
 		}
 
 		$userDataArray = json_decode($result[0]['data'], true);
-		if ($userDataArray === null || json_last_error() !== JSON_ERROR_NONE) {
+		$jsonError = json_last_error();
+		if ($userDataArray === null || $jsonError !== JSON_ERROR_NONE) {
+			$this->logger->critical("User data of $uid contained invalid JSON (error $jsonError), hence falling back to a default user record");
 			return $this->buildDefaultUserRecord($user);
 		}
 
diff --git a/lib/private/Accounts/Hooks.php b/lib/private/Accounts/Hooks.php
index 5c6826a6f9..268f9d8275 100644
--- a/lib/private/Accounts/Hooks.php
+++ b/lib/private/Accounts/Hooks.php
@@ -87,12 +87,8 @@ class Hooks {
 	 * @return AccountManager
 	 */
 	protected function getAccountManager() {
-		if (is_null($this->accountManager)) {
-			$this->accountManager = new AccountManager(
-				\OC::$server->getDatabaseConnection(),
-				\OC::$server->getEventDispatcher(),
-				\OC::$server->getJobList()
-			);
+		if ($this->accountManager === null) {
+			$this->accountManager = \OC::$server->query(AccountManager::class);
 		}
 		return $this->accountManager;
 	}
diff --git a/tests/lib/Accounts/AccountsManagerTest.php b/tests/lib/Accounts/AccountsManagerTest.php
index d727f05d1e..958b6fd473 100644
--- a/tests/lib/Accounts/AccountsManagerTest.php
+++ b/tests/lib/Accounts/AccountsManagerTest.php
@@ -26,7 +26,9 @@ use OC\Accounts\Account;
 use OC\Accounts\AccountManager;
 use OCP\Accounts\IAccountManager;
 use OCP\BackgroundJob\IJobList;
+use OCP\ILogger;
 use OCP\IUser;
+use PHPUnit\Framework\MockObject\MockObject;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\EventDispatcher\GenericEvent;
 use Test\TestCase;
@@ -42,21 +44,24 @@ class AccountsManagerTest extends TestCase {
 	/** @var  \OCP\IDBConnection */
 	private $connection;
 
-	/** @var  EventDispatcherInterface | \PHPUnit_Framework_MockObject_MockObject */
+	/** @var  EventDispatcherInterface|MockObject */
 	private $eventDispatcher;
 
-	/** @var  IJobList | \PHPUnit_Framework_MockObject_MockObject */
+	/** @var  IJobList|MockObject */
 	private $jobList;
 
 	/** @var string accounts table name */
 	private $table = 'accounts';
 
+	/** @var ILogger|MockObject */
+	private $logger;
+
 	protected function setUp(): void {
 		parent::setUp();
-		$this->eventDispatcher = $this->getMockBuilder('Symfony\Component\EventDispatcher\EventDispatcherInterface')
-			->disableOriginalConstructor()->getMock();
+		$this->eventDispatcher = $this->createMock(EventDispatcherInterface::class);
 		$this->connection = \OC::$server->getDatabaseConnection();
-		$this->jobList = $this->getMockBuilder(IJobList::class)->getMock();
+		$this->jobList = $this->createMock(IJobList::class);
+		$this->logger = $this->createMock(ILogger::class);
 	}
 
 	protected function tearDown(): void {
@@ -69,11 +74,11 @@ class AccountsManagerTest extends TestCase {
 	 * get a instance of the accountManager
 	 *
 	 * @param array $mockedMethods list of methods which should be mocked
-	 * @return \PHPUnit_Framework_MockObject_MockObject | AccountManager
+	 * @return MockObject | AccountManager
 	 */
 	public function getInstance($mockedMethods = null) {
 		return $this->getMockBuilder(AccountManager::class)
-			->setConstructorArgs([$this->connection, $this->eventDispatcher, $this->jobList])
+			->setConstructorArgs([$this->connection, $this->eventDispatcher, $this->jobList, $this->logger])
 			->setMethods($mockedMethods)
 			->getMock();