simplify entrypoint for GroupManager

- client does not need to have insights - limit visibility of methods - remove unnecessary AddUserToGroupException exception - only DB issues expected, nothing to catch for us - saves from awkward handling Signed-off-by: Arthur Schiwon <[email protected]>
nextcloud · Jan 18, 2023 · 16a67a1 · 16a67a1
1 parent 22b6b02
commit 16a67a1
Show file tree

Hide file tree

Showing 5 changed files with 33 additions and 63 deletions.
diff --git a/lib/Exceptions/AddUserToGroupException.php b/lib/Exceptions/AddUserToGroupException.php
diff --git a/lib/GroupBackend.php b/lib/GroupBackend.php
@@ -30,7 +30,7 @@
 
 use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
 use Doctrine\DBAL\FetchMode;
-use OCA\User_SAML\Exceptions\AddUserToGroupException;
+use OCP\DB\Exception;
 use OCP\Group\Backend\ABackend;
 use OCP\Group\Backend\IAddToGroupBackend;
 use OCP\Group\Backend\ICountUsersBackend;
@@ -213,17 +213,20 @@ public function createGroup(string $gid, string $samlGid = null): bool {
 		return $result === 1;
 	}
 
+	/**
+	 * @throws Exception
+	 */
 	public function addToGroup(string $uid, string $gid): bool {
-		try {
-			$qb = $this->dbc->getQueryBuilder();
-			$qb->insert(self::TABLE_MEMBERS)
-				->setValue('uid', $qb->createNamedParameter($uid))
-				->setValue('gid', $qb->createNamedParameter($gid))
-				->executeStatement();
+		if ($this->inGroup($uid, $gid)) {
 			return true;
-		} catch (\Exception $e) {
-			throw new AddUserToGroupException($e->getMessage());
 		}
+
+		$qb = $this->dbc->getQueryBuilder();
+		$qb->insert(self::TABLE_MEMBERS)
+			->setValue('uid', $qb->createNamedParameter($uid))
+			->setValue('gid', $qb->createNamedParameter($gid))
+			->executeStatement();
+		return true;
 	}
 
 	public function removeFromGroup(string $uid, string $gid): bool {

diff --git a/lib/GroupManager.php b/lib/GroupManager.php
@@ -119,11 +119,16 @@ private function getGroupsToAdd(array $samlGroupNames, array $assignedGroupIds):
 		return $groupsToAdd;
 	}
 
-	public function replaceGroups(string $uid, array $samlGroupNames): void {
-		$user = $this->userManager->get($uid);
-		if ($user === null) {
-			return;
+	public function handleIncomingGroups(IUser $user, array $samlGroupNames): void {
+		$this->replaceGroups($user, $samlGroupNames);
+		// TODO: drop following line with dropping NC 23 support
+		$this->evaluateGroupMigrations($samlGroupNames);
+		if (isset($e)) {
+			throw $e;
 		}
+	}
+
+	public function replaceGroups(IUser $user, array $samlGroupNames): void {
 		$this->translateGroupToIds($samlGroupNames);
 		$assignedGroups = $this->groupManager->getUserGroups($user);
 		$assignedGroupIds = array_map(function (IGroup $group) {
@@ -144,13 +149,13 @@ protected function translateGroupToIds(array &$samlGroups): void {
 		});
 	}
 
-	public function removeGroups(IUser $user, array $groupIds): void {
+	protected function removeGroups(IUser $user, array $groupIds): void {
 		foreach ($groupIds as $gid) {
 			$this->removeGroup($user, $gid);
 		}
 	}
 
-	public function removeGroup(IUser $user, string $gid): void {
+	protected function removeGroup(IUser $user, string $gid): void {
 		$group = $this->groupManager->get($gid);
 		if ($group === null) {
 			return;
@@ -168,13 +173,13 @@ public function removeGroup(IUser $user, string $gid): void {
 		}
 	}
 
-	public function addGroups(IUser $user, $groupIds): void {
+	protected function addGroups(IUser $user, $groupIds): void {
 		foreach ($groupIds as $gid) {
 			$this->addGroup($user, $gid);
 		}
 	}
 
-	public function addGroup(IUser $user, string $gid): void {
+	protected function addGroup(IUser $user, string $gid): void {
 		try {
 			$group = $this->findGroup($gid);
 		} catch (GroupNotFoundException $e) {
@@ -205,6 +210,9 @@ protected function createGroupInBackend(string $gid, ?string $originalGid = null
 		return $group;
 	}
 
+	/**
+	 * @throws GroupNotFoundException|NonMigratableGroupException
+	 */
 	protected function findGroup(string $gid): IGroup {
 		$migrationAllowList = $this->config->getAppValue(
 			'user_saml',

diff --git a/lib/UserBackend.php b/lib/UserBackend.php
@@ -21,7 +21,6 @@
 
 namespace OCA\User_SAML;
 
-use OCA\User_SAML\Exceptions\AddUserToGroupException;
 use OCP\Authentication\IApacheBackend;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\Files\NotPermittedException;
@@ -665,15 +664,7 @@ public function updateAttributes($uid,
 				$user->setQuota($newQuota);
 			}
 
-			try {
-				$this->groupManager->replaceGroups($user->getUID(), $newGroups ?? []);
-			} catch (AddUserToGroupException $e) {
-				$this->logger->error('Failed to add user to group: {exception}', ['app' => 'user_saml', 'exception' => $e->getMessage()]);
-			}
-			// TODO: drop following line with dropping NC 23 support
-			if (is_array($newGroups)) {
-				$this->groupManager->evaluateGroupMigrations($newGroups);
-			}
+			$this->groupManager->handleIncomingGroups($user, $newGroups ?? []);
 		}
 	}
 

diff --git a/tests/unit/GroupManagerTest.php b/tests/unit/GroupManagerTest.php
@@ -176,7 +176,7 @@ public function testRemoveGroups() {
 			->expects($this->once())
 			->method('deleteGroup');
 
-		$this->ownGroupManager->removeGroups($user, ['groupA']);
+		$this->invokePrivate($this->ownGroupManager, 'removeGroups', [['groupA']]);
 	}
 
 	public function testAddToExistingGroup() {
@@ -202,7 +202,7 @@ public function testAddToExistingGroup() {
 			->expects($this->never())
 			->method('createGroupInBackend');
 
-		$this->ownGroupManager->addGroups($user, ['groupA']);
+		$this->invokePrivate($this->ownGroupManager, 'addGroups', [['groupA']]);
 	}
 
 	public function testAddToNonExistingGroups() {
@@ -232,7 +232,7 @@ public function testAddToNonExistingGroups() {
 			->method('addUser')
 			->with($user);
 
-		$this->ownGroupManager->addGroups($user, ['groupB']);
+		$this->invokePrivate($this->ownGroupManager, 'addGroups', [['groupB']]);
 	}
 
 	public function testAddGroupsWithCollision() {
@@ -275,6 +275,6 @@ public function testAddGroupsWithCollision() {
 			->method('addUser')
 			->with($user);
 
-		$this->ownGroupManager->addGroups($user, ['groupC']);
+		$this->invokePrivate($this->ownGroupManager, 'addGroups', [['groupC']]);
 	}
 }