From fbc5c9b51923356efa010ac813b3af0020aaeb69 Mon Sep 17 00:00:00 2001 From: Giuliano Mele Date: Thu, 14 Jul 2022 12:54:54 +0200 Subject: [PATCH] Fix php code styles Signed-off-by: Giuliano Mele --- appinfo/app.php | 22 +- lib/GroupBackend.php | 8 +- lib/GroupDuplicateChecker.php | 3 +- lib/GroupManager.php | 47 ++-- lib/Jobs/MigrateGroups.php | 21 +- ...emberLocalGroupsForPotentialMigrations.php | 5 +- lib/UserBackend.php | 1 - tests/unit/GroupBackendTest.php | 218 +++++++++--------- tests/unit/GroupManagerTest.php | 3 +- tests/unit/UserBackendTest.php | 1 - 10 files changed, 161 insertions(+), 168 deletions(-) diff --git a/appinfo/app.php b/appinfo/app.php index c2ba235d3..28db1d118 100644 --- a/appinfo/app.php +++ b/appinfo/app.php @@ -49,12 +49,12 @@ return; } -\OC::$server->registerService(GroupDuplicateChecker::class, function(ContainerInterface $c) use($config) { - return new GroupDuplicateChecker( - $config, - $c->get(IGroupManager::class), - $c->get(LoggerInterface::class) - ); +\OC::$server->registerService(GroupDuplicateChecker::class, function (ContainerInterface $c) use ($config) { + return new GroupDuplicateChecker( + $config, + $c->get(IGroupManager::class), + $c->get(LoggerInterface::class) + ); }); $groupBackend = new \OCA\User_SAML\GroupBackend(\OC::$server->getDatabaseConnection()); @@ -62,17 +62,17 @@ $samlSettings = \OC::$server->get(\OCA\User_SAML\SAMLSettings::class); -\OC::$server->registerService(GroupManager::class, function(ContainerInterface $c) use($groupBackend, $samlSettings) { - return new GroupManager( - $c->get(IDBConnection::class), - $c->get(SAMLGroupDuplicateChecker::class), +\OC::$server->registerService(GroupManager::class, function (ContainerInterface $c) use ($groupBackend, $samlSettings) { + return new GroupManager( + $c->get(IDBConnection::class), + $c->get(SAMLGroupDuplicateChecker::class), $c->get(IGroupManager::class), $c->get(IUserManager::class), $groupBackend, $c->get(IConfig::class), $c->get(IJobList::class), $samlSettings, - ); + ); }); $userData = new \OCA\User_SAML\UserData( diff --git a/lib/GroupBackend.php b/lib/GroupBackend.php index 577736b2c..7d01d3af2 100644 --- a/lib/GroupBackend.php +++ b/lib/GroupBackend.php @@ -43,8 +43,8 @@ class GroupBackend extends ABackend implements IAddToGroupBackend, ICountUsersBa /** @var array */ private $groupCache = []; - const TABLE_GROUPS = 'user_saml_groups'; - const TABLE_MEMBERS = 'user_saml_group_members'; + public const TABLE_GROUPS = 'user_saml_groups'; + public const TABLE_MEMBERS = 'user_saml_group_members'; public function __construct(IDBConnection $dbc) { $this->dbc = $dbc; @@ -75,7 +75,7 @@ public function getUserGroups($uid) { ->execute(); $groups = []; - while( $row = $cursor->fetch()) { + while ($row = $cursor->fetch()) { $groups[] = $row['gid']; $this->groupCache[$row['gid']] = $row['gid']; } @@ -195,7 +195,7 @@ public function createGroup(string $gid, string $samlGid = null): bool { ->setValue('displayname', $builder->createNamedParameter($displayName)) ->setValue('saml_gid', $builder->createNamedParameter($samlGid)) ->execute(); - } catch(UniqueConstraintViolationException $e) { + } catch (UniqueConstraintViolationException $e) { $result = 0; } diff --git a/lib/GroupDuplicateChecker.php b/lib/GroupDuplicateChecker.php index da6197c80..3dfa5e2d7 100644 --- a/lib/GroupDuplicateChecker.php +++ b/lib/GroupDuplicateChecker.php @@ -32,8 +32,7 @@ use OCP\IGroupManager; use Psr\Log\LoggerInterface; -class GroupDuplicateChecker -{ +class GroupDuplicateChecker { /** * @var IConfig */ diff --git a/lib/GroupManager.php b/lib/GroupManager.php index 17702ceb3..102f95a22 100644 --- a/lib/GroupManager.php +++ b/lib/GroupManager.php @@ -30,9 +30,7 @@ use OC\BackgroundJob\JobList; use OC\Hooks\PublicEmitter; -use OCA\User_SAML\GroupBackend; use OCA\User_SAML\Jobs\MigrateGroups; -use OCA\User_SAML\SAMLSettings; use OCP\IConfig; use OCP\IDBConnection; use OCP\IGroup; @@ -40,9 +38,8 @@ use OCP\IUser; use OCP\IUserManager; -class GroupManager -{ - const LOCAL_GROUPS_CHECK_FOR_MIGRATION = 'localGroupsCheckForMigration'; +class GroupManager { + public const LOCAL_GROUPS_CHECK_FOR_MIGRATION = 'localGroupsCheckForMigration'; /** * @var IDBConnection $db @@ -89,7 +86,7 @@ public function __construct( private function getGroupsToRemove(array $samlGroups, array $assignedGroups): array { $groupsToRemove = []; - foreach($assignedGroups as $group) { + foreach ($assignedGroups as $group) { // if group is not supplied by SAML and group has SAML backend if (!in_array($group->getGID(), $samlGroups) && $this->hasSamlBackend($group)) { $groupsToRemove[] = $group->getGID(); @@ -100,7 +97,7 @@ private function getGroupsToRemove(array $samlGroups, array $assignedGroups): ar private function getGroupsToAdd(array $samlGroups, array $assignedGroupIds): array { $groupsToAdd = []; - foreach($samlGroups as $group) { + foreach ($samlGroups as $group) { // if user is not assigend to the group or the provided group has a non SAML backend if (!in_array($group, $assignedGroupIds) || !$this->hasSamlBackend($this->groupManager->get($group))) { $groupsToAdd[] = $group; @@ -111,12 +108,12 @@ private function getGroupsToAdd(array $samlGroups, array $assignedGroupIds): arr public function replaceGroups(string $uid, array $samlGroups): void { $user = $this->userManager->get($uid); - if($user === null) { + if ($user === null) { return; } $this->translateGroupToIds($samlGroups); $assignedGroups = $this->groupManager->getUserGroups($user); - $assignedGroupIds = array_map(function(IGroup $group){ + $assignedGroupIds = array_map(function (IGroup $group) { return $group->getGID(); }, $assignedGroups); $groupsToRemove = $this->getGroupsToRemove($samlGroups, $assignedGroups); @@ -126,9 +123,9 @@ public function replaceGroups(string $uid, array $samlGroups): void { } protected function translateGroupToIds(array &$samlGroups): void { - array_walk($samlGroups, function (&$gid){ + array_walk($samlGroups, function (&$gid) { $altGid = $this->ownGroupBackend->groupExistsWithDifferentGid($gid); - if($altGid !== null) { + if ($altGid !== null) { $gid = $altGid; } }); @@ -142,7 +139,7 @@ public function removeGroups(IUser $user, array $groupIds): void { public function removeGroup(IUser $user, string $gid): void { $group = $this->groupManager->get($gid); - if($group === null) { + if ($group === null) { return; } $this->ownGroupBackend->removeFromGroup($user->getUID(), $group->getGID()); @@ -161,9 +158,9 @@ public function addGroup(IUser $user, string $gid): void { try { $group = $this->findGroup($gid); } catch (\RuntimeException $e) { - if($e->getCode() === 1) { + if ($e->getCode() === 1) { $group = $this->createGroupInBackend($gid); - } else if($e->getCode() === 2) { + } elseif ($e->getCode() === 2) { //FIXME: probably need config flag. Previous to 17, gid was used as displayname $providerId = $this->settings->getProviderId(); $settings = $this->settings->get($providerId); @@ -179,15 +176,15 @@ public function addGroup(IUser $user, string $gid): void { } protected function createGroupInBackend(string $gid, ?string $originalGid = null): ?IGroup { - if($this->groupManager instanceof PublicEmitter) { + if ($this->groupManager instanceof PublicEmitter) { $this->groupManager->emit('\OC\Group', 'preCreate', array($gid)); } - if(!$this->ownGroupBackend->createGroup($gid, $originalGid ?? $gid)) { + if (!$this->ownGroupBackend->createGroup($gid, $originalGid ?? $gid)) { return null; } $group = $this->groupManager->get($gid); - if($this->groupManager instanceof PublicEmitter) { + if ($this->groupManager instanceof PublicEmitter) { $this->groupManager->emit('\OC\Group', 'postCreate', array($group)); } @@ -204,25 +201,25 @@ protected function findGroup(string $gid): IGroup { if ($migrationWhiteList !== null) { $migrationWhiteList = \json_decode($migrationWhiteList, true); } - if(!$strictBackendCheck && in_array($gid, $migrationWhiteList['groups'], true)) { + if (!$strictBackendCheck && in_array($gid, $migrationWhiteList['groups'], true)) { $group = $this->groupManager->get($gid); - if($group === null) { + if ($group === null) { //FIXME: specific Exception and/or constant error code throw new \RuntimeException('Group not found', 1); } return $group; } $group = $this->groupManager->get($gid); - if($group === null) { + if ($group === null) { //FIXME: specific Exception and/or constant error code throw new \RuntimeException('Group not found', 1); } - if($this->hasSamlBackend($group)) { + if ($this->hasSamlBackend($group)) { return $group; } $altGid = $this->ownGroupBackend->groupExistsWithDifferentGid($gid); - if($altGid) { + if ($altGid) { return $this->groupManager->get($altGid); } @@ -238,7 +235,7 @@ protected function hasSamlBackend(IGroup $group): bool { // available at nextcloud 22 // $backends = $group->getBackendNames(); foreach ($backends as $backend) { - if($backend instanceof GroupBackend) { + if ($backend instanceof GroupBackend) { return true; } } @@ -247,11 +244,11 @@ protected function hasSamlBackend(IGroup $group): bool { public function evaluateGroupMigrations(array $groups): void { $candidateInfo = $this->config->getAppValue('user_saml', self::LOCAL_GROUPS_CHECK_FOR_MIGRATION, null); - if($candidateInfo === null) { + if ($candidateInfo === null) { return; } $candidateInfo = \json_decode($candidateInfo, true); - if(!isset($candidateInfo['dropAfter']) || $candidateInfo['dropAfter'] < time()) { + if (!isset($candidateInfo['dropAfter']) || $candidateInfo['dropAfter'] < time()) { $this->config->deleteAppValue('user_saml', self::LOCAL_GROUPS_CHECK_FOR_MIGRATION); return; } diff --git a/lib/Jobs/MigrateGroups.php b/lib/Jobs/MigrateGroups.php index e2805af7c..d0b0607cf 100644 --- a/lib/Jobs/MigrateGroups.php +++ b/lib/Jobs/MigrateGroups.php @@ -1,4 +1,5 @@ @@ -79,11 +80,11 @@ protected function run($argument) { protected function updateCandidatePool($migrateGroups) { $candidateInfo = $this->config->getAppValue('user_saml', GroupManager::LOCAL_GROUPS_CHECK_FOR_MIGRATION, null); - if($candidateInfo === null) { + if ($candidateInfo === null) { return; } $candidateInfo = \json_decode($candidateInfo, true); - if(!isset($candidateInfo['dropAfter']) || !isset($candidateInfo['groups'])) { + if (!isset($candidateInfo['dropAfter']) || !isset($candidateInfo['groups'])) { return; } $candidateInfo['groups'] = array_diff($candidateInfo['groups'], $migrateGroups); @@ -108,11 +109,11 @@ protected function migrateGroup(string $gid): bool { $affected = $qb->delete('groups') ->where($qb->expr()->eq('gid', $qb->createNamedParameter($gid))) ->execute(); - if($affected === 0) { + if ($affected === 0) { throw new \RuntimeException('Could not delete group from local backend'); } - if(!$this->ownGroupBackend->createGroup($gid)) { + if (!$this->ownGroupBackend->createGroup($gid)) { throw new \RuntimeException('Could not create group in SAML backend'); } @@ -129,12 +130,12 @@ protected function migrateGroup(string $gid): bool { protected function getGroupsToMigrate(array $samlGroups, array $pool): array { return array_filter($samlGroups, function (string $gid) use ($pool) { - if(!in_array($gid, $pool)) { + if (!in_array($gid, $pool)) { return false; } $group = $this->groupManager->get($gid); - if($group === null) { + if ($group === null) { return false; } $reflected = new \ReflectionClass($group); @@ -142,7 +143,7 @@ protected function getGroupsToMigrate(array $samlGroups, array $pool): array { $backendsProperty->setAccessible(true); $backends = $backendsProperty->getValue($group); foreach ($backends as $backend) { - if($backend instanceof Database) { + if ($backend instanceof Database) { return true; } } @@ -152,17 +153,15 @@ protected function getGroupsToMigrate(array $samlGroups, array $pool): array { protected function getMigratableGroups(): array { $candidateInfo = $this->config->getAppValue('user_saml', GroupManager::LOCAL_GROUPS_CHECK_FOR_MIGRATION, null); - if($candidateInfo === null) { + if ($candidateInfo === null) { throw new \RuntimeException('No migration of groups to SAML backend anymore'); } $candidateInfo = \json_decode($candidateInfo, true); - if(!isset($candidateInfo['dropAfter']) || !isset($candidateInfo['groups']) || $candidateInfo['dropAfter'] < time()) { + if (!isset($candidateInfo['dropAfter']) || !isset($candidateInfo['groups']) || $candidateInfo['dropAfter'] < time()) { $this->config->deleteAppValue('user_saml', GroupManager::LOCAL_GROUPS_CHECK_FOR_MIGRATION); throw new \RuntimeException('Period for migration groups is over'); } return $candidateInfo['groups']; } - - } diff --git a/lib/Migration/RememberLocalGroupsForPotentialMigrations.php b/lib/Migration/RememberLocalGroupsForPotentialMigrations.php index 7294bf7a1..9bc9e81b4 100644 --- a/lib/Migration/RememberLocalGroupsForPotentialMigrations.php +++ b/lib/Migration/RememberLocalGroupsForPotentialMigrations.php @@ -1,4 +1,5 @@ @@ -86,7 +87,7 @@ public function run(IOutput $output) { protected function findGroupIds(Database $backend): array { $groupIds = $backend->getGroups(); $adminGroupIndex = array_search('admin', $groupIds, true); - if($adminGroupIndex !== false) { + if ($adminGroupIndex !== false) { unset($groupIds[$adminGroupIndex]); } return $groupIds; @@ -95,7 +96,7 @@ protected function findGroupIds(Database $backend): array { protected function findBackend(): Database { $groupBackends = $this->groupManager->getBackends(); foreach ($groupBackends as $backend) { - if($backend instanceof Database) { + if ($backend instanceof Database) { return $backend; break; } diff --git a/lib/UserBackend.php b/lib/UserBackend.php index e4e369f4c..ce1df929a 100644 --- a/lib/UserBackend.php +++ b/lib/UserBackend.php @@ -29,7 +29,6 @@ use OCP\ILogger; use OCP\IUser; use OCP\IUserManager; -use OCA\User_SAML\GroupManager; use OCP\UserInterface; use OCP\IUserBackend; use OCP\IConfig; diff --git a/tests/unit/GroupBackendTest.php b/tests/unit/GroupBackendTest.php index 3aa186688..ba584fa32 100644 --- a/tests/unit/GroupBackendTest.php +++ b/tests/unit/GroupBackendTest.php @@ -34,119 +34,119 @@ */ class GroupBackendTest extends TestCase { - /** @var GroupBackend */ - private static $groupBackend; - private static $users = [ - [ - 'uid' => 'user_saml_integration_test_uid1', - 'groups' => [ - 'user_saml_integration_test_gid1', - 'SAML_user_saml_integration_test_gid2' - ] - ], - [ - 'uid' => 'user_saml_integration_test_uid2', - 'groups' => [ - 'user_saml_integration_test_gid1' - ] - ] - ]; - private static $groups = [ - [ - 'gid' => 'user_saml_integration_test_gid1', - 'saml_gid' => 'user_saml_integration_test_gid1', - 'members' => [ - 'user_saml_integration_test_uid1', - 'user_saml_integration_test_uid2' - ], - 'saml_gid_exists' => true - ], - [ - 'gid' => 'SAML_user_saml_integration_test_gid2', - 'saml_gid' => 'user_saml_integration_test_gid2', - 'members' => [ - 'user_saml_integration_test_uid1' - ], - 'saml_gid_exists' => false - ], - [ - 'gid' => 'user_saml_integration_test_gid3', - 'saml_gid' => 'user_saml_integration_test_gid3', - 'members' => [], - 'saml_gid_exists' => true - ], - ]; + /** @var GroupBackend */ + private static $groupBackend; + private static $users = [ + [ + 'uid' => 'user_saml_integration_test_uid1', + 'groups' => [ + 'user_saml_integration_test_gid1', + 'SAML_user_saml_integration_test_gid2' + ] + ], + [ + 'uid' => 'user_saml_integration_test_uid2', + 'groups' => [ + 'user_saml_integration_test_gid1' + ] + ] + ]; + private static $groups = [ + [ + 'gid' => 'user_saml_integration_test_gid1', + 'saml_gid' => 'user_saml_integration_test_gid1', + 'members' => [ + 'user_saml_integration_test_uid1', + 'user_saml_integration_test_uid2' + ], + 'saml_gid_exists' => true + ], + [ + 'gid' => 'SAML_user_saml_integration_test_gid2', + 'saml_gid' => 'user_saml_integration_test_gid2', + 'members' => [ + 'user_saml_integration_test_uid1' + ], + 'saml_gid_exists' => false + ], + [ + 'gid' => 'user_saml_integration_test_gid3', + 'saml_gid' => 'user_saml_integration_test_gid3', + 'members' => [], + 'saml_gid_exists' => true + ], + ]; - 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']); - } - foreach(self::$users as $user){ - foreach($user['groups'] as $group){ - self::$groupBackend->addToGroup($user['uid'], $group); - } - } - } + 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']); + } + foreach (self::$users as $user) { + foreach ($user['groups'] as $group) { + self::$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){ - foreach($user['groups'] as $group){ - self::$groupBackend->removeFromGroup($user['uid'], $group); - } - } - foreach (self::$groups as $group) { - self::$groupBackend->deleteGroup($group['gid']); - } - } + public static function tearDownAfterClass(): void { + parent::tearDownAfterClass(); + self::$groupBackend = new \OCA\User_SAML\GroupBackend(\OC::$server->getDatabaseConnection()); + foreach (self::$users as $user) { + foreach ($user['groups'] as $group) { + self::$groupBackend->removeFromGroup($user['uid'], $group); + } + } + foreach (self::$groups as $group) { + self::$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']); - if(in_array($group['gid'], $user['groups'])){ - $this->assertTrue($result, sprintf("User %s should be member of group %s", $user['uid'], $group['gid'])); - } else { - $this->assertFalse($result, sprintf("User %s should not be member of group %s", $user['uid'], $group['gid'])); - } - } - } - } + public function testInGroup() { + foreach (self::$groups as $group) { + foreach (self::$users as $user) { + $result = self::$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 { + $this->assertFalse($result, sprintf("User %s should not be member of group %s", $user['uid'], $group['gid'])); + } + } + } + } - public function testGetGroups() { - $groups = self::$groupBackend->getGroups(); - foreach (self::$groups as $group) { - $this->assertContains($group['gid'], $groups, sprintf('Group %s should be retrieved', $group['gid'])); - } - } + public function testGetGroups() { + $groups = self::$groupBackend->getGroups(); + foreach (self::$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']); - $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)); - } - } - } + public function testGetUserGroups() { + foreach (self::$users as $user) { + $userGroups = self::$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)); + } + } + } - public function testGroupExists() { - foreach(self::$groups as $group){ - $result = self::$groupBackend->groupExists($group['saml_gid']); - $this->assertSame($group['saml_gid_exists'], $result, sprintf('Group %s should exist', $group['saml_gid'])); - } - } + public function testGroupExists() { + foreach (self::$groups as $group) { + $result = self::$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']); - $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'])); - } - } - } -} \ No newline at end of file + public function testUsersInGroups() { + foreach (self::$groups as $group) { + $users = self::$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'])); + } + } + } +} diff --git a/tests/unit/GroupManagerTest.php b/tests/unit/GroupManagerTest.php index 9bf64fd0d..9f9165f4f 100644 --- a/tests/unit/GroupManagerTest.php +++ b/tests/unit/GroupManagerTest.php @@ -1,7 +1,7 @@ - * + * * @author Giuliano Mele * * @license GNU AGPL version 3 or any later version @@ -276,5 +276,4 @@ public function testAddGroupsWithCollision() { $this->ownGroupManager->addGroups($user, ['groupC']); } - } diff --git a/tests/unit/UserBackendTest.php b/tests/unit/UserBackendTest.php index 74140a756..ff16b94ee 100644 --- a/tests/unit/UserBackendTest.php +++ b/tests/unit/UserBackendTest.php @@ -27,7 +27,6 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; use OCP\IDBConnection; -use OCP\IGroup; use OCA\User_SAML\GroupManager; use OCP\ILogger; use OCP\ISession;