refactor(settings): Modernize authorized group classes

Signed-off-by: provokateurin <kate@provokateurin.de>

apps/settings/lib/Service/AuthorizedGroupService.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2021 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
@@ -13,9 +15,9 @@ use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Db\MultipleObjectsReturnedException;
 use OCP\DB\Exception;
 use OCP\IGroup;
+use Throwable;
 
-class AuthorizedGroupService {
-
+readonly class AuthorizedGroupService {
 	public function __construct(
 		private AuthorizedGroupMapper $mapper,
 	) {
@@ -23,6 +25,7 @@ class AuthorizedGroupService {
 
 	/**
 	 * @return AuthorizedGroup[]
+	 * @throws Exception
 	 */
 	public function findAll(): array {
 		return $this->mapper->findAll();
@@ -30,43 +33,40 @@ class AuthorizedGroupService {
 
 	/**
 	 * Find AuthorizedGroup by id.
-	 *
-	 * @param int $id
+	 * @throws DoesNotExistException
+	 * @throws Exception
+	 * @throws MultipleObjectsReturnedException
 	 */
 	public function find(int $id): ?AuthorizedGroup {
 		return $this->mapper->find($id);
 	}
 
 	/**
-	 * @param $e
 	 * @throws NotFoundException
+	 * @throws Throwable
 	 */
-	private function handleException(\Exception $e): void {
+	private function handleException(Throwable $e): void {
 		if ($e instanceof DoesNotExistException
 			|| $e instanceof MultipleObjectsReturnedException) {
 			throw new NotFoundException('AuthorizedGroup not found');
-		} else {
-			throw $e;
 		}
+
+		throw $e;
 	}
 
 	/**
 	 * Create a new AuthorizedGroup
 	 *
-	 * @param string $groupId
-	 * @param string $class
-	 * @return AuthorizedGroup
 	 * @throws Exception
 	 * @throws ConflictException
+	 * @throws MultipleObjectsReturnedException
 	 */
 	public function create(string $groupId, string $class): AuthorizedGroup {
 		// Check if the group is already assigned to this class
 		try {
-			$existing = $this->mapper->findByGroupIdAndClass($groupId, $class);
-			if ($existing) {
-				throw new ConflictException('Group is already assigned to this class');
-			}
-		} catch (DoesNotExistException $e) {
+			$this->mapper->findByGroupIdAndClass($groupId, $class);
+			throw new ConflictException('Group is already assigned to this class');
+		} catch (DoesNotExistException) {
 			// This is expected when no duplicate exists, continue with creation
 		}
 
@@ -78,30 +78,37 @@ class AuthorizedGroupService {
 
 	/**
 	 * @throws NotFoundException
+	 * @throws Throwable
 	 */
 	public function delete(int $id): void {
 		try {
 			$authorizedGroup = $this->mapper->find($id);
 			$this->mapper->delete($authorizedGroup);
-		} catch (\Exception $e) {
-			$this->handleException($e);
+		} catch (\Exception $exception) {
+			$this->handleException($exception);
 		}
 	}
 
+	/**
+	 * @return list<AuthorizedGroup>
+	 */
 	public function findExistingGroupsForClass(string $class): array {
 		try {
-			$authorizedGroup = $this->mapper->findExistingGroupsForClass($class);
-			return $authorizedGroup;
-		} catch (\Exception $e) {
+			return $this->mapper->findExistingGroupsForClass($class);
+		} catch (\Exception) {
 			return [];
 		}
 	}
 
+	/**
+	 * @throws Throwable
+	 * @throws NotFoundException
+	 */
 	public function removeAuthorizationAssociatedTo(IGroup $group): void {
 		try {
 			$this->mapper->removeGroup($group->getGID());
-		} catch (\Exception $e) {
-			$this->handleException($e);
+		} catch (\Exception $exception) {
+			$this->handleException($exception);
 		}
 	}
 }

build/rector-strict.php

@@ -14,6 +14,9 @@ return (require __DIR__ . '/rector-shared.php')
 		$nextcloudDir . '/lib/public/IContainer.php',
 		$nextcloudDir . '/apps/dav/lib/Connector/Sabre/Node.php',
 		$nextcloudDir . '/apps/files_versions/lib/Versions/IMetadataVersion.php',
+		$nextcloudDir . '/lib/private/Settings/AuthorizedGroup.php',
+		$nextcloudDir . '/lib/private/Settings/AuthorizedGroupMapper.php',
+		$nextcloudDir . '/apps/settings/lib/Service/AuthorizedGroupService.php',
 	])
 	->withPreparedSets(
 		deadCode: true,

lib/private/Settings/AuthorizedGroup.php

@@ -8,21 +8,25 @@ declare(strict_types=1);
  */
 namespace OC\Settings;
 
+use JsonSerializable;
 use OCP\AppFramework\Db\Entity;
 
 /**
  * @method setGroupId(string $groupId)
  * @method setClass(string $class)
- * @method getGroupId(): string
- * @method getClass(): string
+ * @method string getGroupId()
+ * @method string getClass()
  */
-class AuthorizedGroup extends Entity implements \JsonSerializable {
-	/** @var string $group_id */
-	protected $groupId;
+class AuthorizedGroup extends Entity implements JsonSerializable {
+	public $id;
 
-	/** @var string $class */
-	protected $class;
+	protected ?string $groupId = null;
 
+	protected ?string $class = null;
+
+	/**
+	 * @return array<string, mixed>
+	 */
 	public function jsonSerialize(): array {
 		return [
 			'id' => $this->getId(),

lib/private/Settings/AuthorizedGroupMapper.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2021 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
@@ -7,7 +9,6 @@
 namespace OC\Settings;
 
 use OCP\AppFramework\Db\DoesNotExistException;
-use OCP\AppFramework\Db\Entity;
 use OCP\AppFramework\Db\MultipleObjectsReturnedException;
 use OCP\AppFramework\Db\QBMapper;
 use OCP\DB\Exception;
@@ -32,48 +33,40 @@ class AuthorizedGroupMapper extends QBMapper {
 	public function findAllClassesForUser(IUser $user): array {
 		$qb = $this->db->getQueryBuilder();
 
-		/** @var IGroupManager $groupManager */
 		$groupManager = Server::get(IGroupManager::class);
 		$groups = $groupManager->getUserGroups($user);
 		if (count($groups) === 0) {
 			return [];
 		}
 
-		$result = $qb->select('class')
+		/** @var list<string> $rows */
+		$rows = $qb->select('class')
 			->from($this->getTableName(), 'auth')
-			->where($qb->expr()->in('group_id', array_map(function (IGroup $group) use ($qb) {
-				return $qb->createNamedParameter($group->getGID());
-			}, $groups), IQueryBuilder::PARAM_STR))
-			->executeQuery();
+			->where($qb->expr()->in('group_id', array_map(static fn (IGroup $group) => $qb->createNamedParameter($group->getGID()), $groups), IQueryBuilder::PARAM_STR))
+			->executeQuery()
+			->fetchFirstColumn();
 
-		$classes = [];
-		while ($row = $result->fetch()) {
-			$classes[] = $row['class'];
-		}
-		$result->closeCursor();
-		return $classes;
+		return $rows;
 	}
 
 	/**
 	 * @throws DoesNotExistException
 	 * @throws MultipleObjectsReturnedException
-	 * @throws \OCP\DB\Exception
+	 * @throws Exception
 	 */
 	public function find(int $id): AuthorizedGroup {
 		$queryBuilder = $this->db->getQueryBuilder();
 		$queryBuilder->select('*')
 			->from($this->getTableName())
 			->where($queryBuilder->expr()->eq('id', $queryBuilder->createNamedParameter($id)));
-		/** @var AuthorizedGroup $authorizedGroup */
-		$authorizedGroup = $this->findEntity($queryBuilder);
-		return $authorizedGroup;
+		return $this->findEntity($queryBuilder);
 	}
 
 	/**
 	 * Get all the authorizations stored in the database.
 	 *
 	 * @return AuthorizedGroup[]
-	 * @throws \OCP\DB\Exception
+	 * @throws Exception
 	 */
 	public function findAll(): array {
 		$qb = $this->db->getQueryBuilder();
@@ -81,7 +74,12 @@ class AuthorizedGroupMapper extends QBMapper {
 		return $this->findEntities($qb);
 	}
 
-	public function findByGroupIdAndClass(string $groupId, string $class) {
+	/**
+	 * @throws DoesNotExistException
+	 * @throws Exception
+	 * @throws MultipleObjectsReturnedException
+	 */
+	public function findByGroupIdAndClass(string $groupId, string $class): AuthorizedGroup {
 		$qb = $this->db->getQueryBuilder();
 		$qb->select('*')
 			->from($this->getTableName())
@@ -91,8 +89,8 @@ class AuthorizedGroupMapper extends QBMapper {
 	}
 
 	/**
-	 * @return Entity[]
-	 * @throws \OCP\DB\Exception
+	 * @return list<AuthorizedGroup>
+	 * @throws Exception
 	 */
 	public function findExistingGroupsForClass(string $class): array {
 		$qb = $this->db->getQueryBuilder();
@@ -105,7 +103,7 @@ class AuthorizedGroupMapper extends QBMapper {
 	/**
 	 * @throws Exception
 	 */
-	public function removeGroup(string $gid) {
+	public function removeGroup(string $gid): void {
 		$qb = $this->db->getQueryBuilder();
 		$qb->delete($this->getTableName())
 			->where($qb->expr()->eq('group_id', $qb->createNamedParameter($gid)))

psalm-strict.xml

@@ -20,6 +20,9 @@
 		<file name="lib/public/IContainer.php"/>
 		<file name="apps/dav/lib/Connector/Sabre/Node.php"/>
 		<file name="apps/files_versions/lib/Versions/IMetadataVersion.php"/>
+		<file name="lib/private/Settings/AuthorizedGroup.php"/>
+		<file name="lib/private/Settings/AuthorizedGroupMapper.php"/>
+		<file name="apps/settings/lib/Service/AuthorizedGroupService.php"/>
 		<ignoreFiles>
 			<directory name="apps/**/composer"/>
 			<directory name="apps/**/tests"/>
@@ -30,6 +33,7 @@
 	</projectFiles>
 	<extraFiles>
 		<directory name="apps/dav/lib"/>
+		<directory name="apps/settings/lib"/>
 		<directory name="3rdparty"/>
 	</extraFiles>
 	<stubs>