Merge pull request #839 from nextcloud/fix/838/result-undefined

fix(Groups): take other DB errors into consideration

Author: juliusknorr

lib/GroupBackend.php

@@ -38,19 +38,20 @@
 use OCP\Group\Backend\INamedBackend;
 use OCP\Group\Backend\IRemoveFromGroupBackend;
 use OCP\IDBConnection;
+use Psr\Log\LoggerInterface;
 
 class GroupBackend extends ABackend implements IAddToGroupBackend, ICountUsersBackend, ICreateGroupBackend, IDeleteGroupBackend, IGetDisplayNameBackend, IRemoveFromGroupBackend, INamedBackend {
-	/** @var IDBConnection */
-	private $dbc;
 
 	/** @var array  */
 	private $groupCache = [];
 
 	public const TABLE_GROUPS = 'user_saml_groups';
 	public const TABLE_MEMBERS = 'user_saml_group_members';
 
-	public function __construct(IDBConnection $dbc) {
-		$this->dbc = $dbc;
+	public function __construct(
+		protected IDBConnection $dbc,
+		protected LoggerInterface $logger
+	) {
 	}
 
 	public function inGroup($uid, $gid): bool {
@@ -208,8 +209,14 @@ public function createGroup(string $gid, string $samlGid = null): bool {
 				->setValue('saml_gid', $builder->createNamedParameter($samlGid))
 				->executeStatement();
 		} catch (Exception $e) {
-			if ($e->getReason() === \OCP\DB\Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
+			if ($e->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
 				$result = 0;
+			} else {
+				$this->logger->warning('Failed to create group: ' . $e->getMessage(), [
+					'app' => 'user_saml',
+					'exception' => $e,
+				]);
+				$result = -1;
 			}
 		}
 

tests/unit/GroupBackendTest.php

@@ -27,6 +27,8 @@
  */
 
 use \OCA\User_SAML\GroupBackend;
+use OCP\IDBConnection;
+use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
 /**
@@ -35,8 +37,8 @@
 class GroupBackendTest extends TestCase {
 
 	/** @var GroupBackend */
-	private static $groupBackend;
-	private static $users = [
+	private $groupBackend;
+	private $users = [
 		[
 			'uid' => 'user_saml_integration_test_uid1',
 			'groups' => [
@@ -51,7 +53,7 @@ class GroupBackendTest extends TestCase {
 			]
 		]
 	];
-	private static $groups = [
+	private $groups = [
 		[
 			'gid' => 'user_saml_integration_test_gid1',
 			'saml_gid' => 'user_saml_integration_test_gid1',
@@ -77,36 +79,36 @@ class GroupBackendTest extends TestCase {
 		],
 	];
 
-	public static function setUpBeforeClass(): void {
-		parent::setUpBeforeClass();
-		self::$groupBackend = new \OCA\User_SAML\GroupBackend(\OC::$server->getDatabaseConnection());
-		foreach (self::$groups as $group) {
-			self::$groupBackend->createGroup($group['gid'], $group['saml_gid']);
+	public function setUp(): void {
+		parent::setUp();
+		$this->groupBackend = new GroupBackend(\OC::$server->get(IDBConnection::class), $this->createMock(LoggerInterface::class));
+		foreach ($this->groups as $group) {
+			$this->groupBackend->createGroup($group['gid'], $group['saml_gid']);
 		}
-		foreach (self::$users as $user) {
+		foreach ($this->users as $user) {
 			foreach ($user['groups'] as $group) {
-				self::$groupBackend->addToGroup($user['uid'], $group);
+				$this->groupBackend->addToGroup($user['uid'], $group);
 			}
 		}
 	}
 
-	public static function tearDownAfterClass(): void {
-		parent::tearDownAfterClass();
-		self::$groupBackend = new \OCA\User_SAML\GroupBackend(\OC::$server->getDatabaseConnection());
-		foreach (self::$users as $user) {
+	public function tearDown(): void {
+		parent::tearDown();
+		$this->groupBackend = new GroupBackend(\OC::$server->get(IDBConnection::class), $this->createMock(LoggerInterface::class));
+		foreach ($this->users as $user) {
 			foreach ($user['groups'] as $group) {
-				self::$groupBackend->removeFromGroup($user['uid'], $group);
+				$this->groupBackend->removeFromGroup($user['uid'], $group);
 			}
 		}
-		foreach (self::$groups as $group) {
-			self::$groupBackend->deleteGroup($group['gid']);
+		foreach ($this->groups as $group) {
+			$this->groupBackend->deleteGroup($group['gid']);
 		}
 	}
 
 	public function testInGroup() {
-		foreach (self::$groups as $group) {
-			foreach (self::$users as $user) {
-				$result = self::$groupBackend->inGroup($user['uid'], $group['gid']);
+		foreach ($this->groups as $group) {
+			foreach ($this->users as $user) {
+				$result = $this->groupBackend->inGroup($user['uid'], $group['gid']);
 				if (in_array($group['gid'], $user['groups'])) {
 					$this->assertTrue($result, sprintf("User %s should be member of group %s", $user['uid'], $group['gid']));
 				} else {
@@ -117,15 +119,15 @@ public function testInGroup() {
 	}
 
 	public function testGetGroups() {
-		$groups = self::$groupBackend->getGroups();
-		foreach (self::$groups as $group) {
+		$groups = $this->groupBackend->getGroups();
+		foreach ($this->groups as $group) {
 			$this->assertContains($group['gid'], $groups, sprintf('Group %s should be retrieved', $group['gid']));
 		}
 	}
 
 	public function testGetUserGroups() {
-		foreach (self::$users as $user) {
-			$userGroups = self::$groupBackend->getUserGroups($user['uid']);
+		foreach ($this->users as $user) {
+			$userGroups = $this->groupBackend->getUserGroups($user['uid']);
 			$this->assertCount(count($user['groups']), $userGroups, 'Should retrieve all user groups');
 			foreach ($userGroups as $userGroup) {
 				$this->assertContains($userGroup, $user['groups'], sprintf('Users %s should be member of groups %s', $user['uid'], $userGroup));
@@ -134,15 +136,15 @@ public function testGetUserGroups() {
 	}
 
 	public function testGroupExists() {
-		foreach (self::$groups as $group) {
-			$result = self::$groupBackend->groupExists($group['saml_gid']);
+		foreach ($this->groups as $group) {
+			$result = $this->groupBackend->groupExists($group['saml_gid']);
 			$this->assertSame($group['saml_gid_exists'], $result, sprintf('Group %s should exist', $group['saml_gid']));
 		}
 	}
 
 	public function testUsersInGroups() {
-		foreach (self::$groups as $group) {
-			$users = self::$groupBackend->usersInGroup($group['gid']);
+		foreach ($this->groups as $group) {
+			$users = $this->groupBackend->usersInGroup($group['gid']);
 			$this->assertCount(count($group['members']), $users, 'Should retrieve all group members');
 			foreach ($users as $user) {
 				$this->assertContains($user, $group['members'], sprintf('User %s should be member of group %s', $user, $group['gid']));