From 56662ca9f92f0994d2c714bcbb4b2d7ffcfab33c Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 12 Aug 2024 17:51:21 +0900 Subject: [PATCH 1/7] feat: add -g option to `shield:user create` --- src/Commands/User.php | 19 ++++++++++++++----- tests/Commands/UserTest.php | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/src/Commands/User.php b/src/Commands/User.php index 939438138..9c1a91019 100644 --- a/src/Commands/User.php +++ b/src/Commands/User.php @@ -53,6 +53,7 @@ class User extends BaseCommand shield:user options shield:user create -n newusername -e newuser@example.com + shield:user create -n newusername -e newuser@example.com -g mygroup shield:user activate -n username shield:user activate -e user@example.com @@ -159,7 +160,7 @@ public function run(array $params): int try { switch ($action) { case 'create': - $this->create($username, $email); + $this->create($username, $email, $group); break; case 'activate': @@ -252,8 +253,9 @@ private function setValidationRules(): void * * @param string|null $username User name to create (optional) * @param string|null $email User email to create (optional) + * @param string|null $group Group to add user to (optional) */ - private function create(?string $username = null, ?string $email = null): void + private function create(?string $username = null, ?string $email = null, ?string $group = null): void { $data = []; @@ -311,11 +313,18 @@ private function create(?string $username = null, ?string $email = null): void $this->write('User "' . $username . '" created', 'green'); } - // Add to default group $user = $userModel->findById($userModel->getInsertID()); - $userModel->addToDefaultGroup($user); - $this->write('The user is added to the default group.', 'green'); + if ($group === null) { + // Add to default group + $userModel->addToDefaultGroup($user); + + $this->write('The user is added to the default group.', 'green'); + } else { + $user->addGroup($group); + + $this->write('The user is added to group "' . $group . '".', 'green'); + } } /** diff --git a/tests/Commands/UserTest.php b/tests/Commands/UserTest.php index a85e713d6..9fdf3e148 100644 --- a/tests/Commands/UserTest.php +++ b/tests/Commands/UserTest.php @@ -100,6 +100,40 @@ public function testCreate(): void ]); } + public function testCreateWithGroupBeta(): void + { + $this->setMockIo([ + 'Secret Passw0rd!', + 'Secret Passw0rd!', + ]); + + command('shield:user create -n user1 -e user1@example.com -g beta'); + + $this->assertStringContainsString( + 'User "user1" created', + $this->io->getFirstOutput() + ); + $this->assertStringContainsString( + 'The user is added to group "beta"', + $this->io->getFirstOutput() + ); + + $users = model(UserModel::class); + $user = $users->findByCredentials(['email' => 'user1@example.com']); + $this->seeInDatabase($this->tables['identities'], [ + 'user_id' => $user->id, + 'secret' => 'user1@example.com', + ]); + $this->seeInDatabase($this->tables['users'], [ + 'id' => $user->id, + 'active' => 0, + ]); + $this->seeInDatabase($this->tables['groups_users'], [ + 'user_id' => $user->id, + 'group' => 'beta', + ]); + } + public function testCreateNotUniqueName(): void { $user = $this->createUser([ From 4cac2d8b7a13a1e89090a1460912af3d1c88cab9 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 20 Aug 2024 14:03:10 +0900 Subject: [PATCH 2/7] refactor: extract isValidGroup() --- src/Authorization/Traits/Authorizable.php | 28 +++++++++++------------ src/Models/UserModel.php | 17 +++++++++++--- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/Authorization/Traits/Authorizable.php b/src/Authorization/Traits/Authorizable.php index 79e313951..dc507cf92 100644 --- a/src/Authorization/Traits/Authorizable.php +++ b/src/Authorization/Traits/Authorizable.php @@ -33,8 +33,6 @@ public function addGroup(string ...$groups): self { $this->populateGroups(); - $configGroups = $this->getConfigGroups(); - $groupCount = count($this->groupCache); foreach ($groups as $group) { @@ -46,7 +44,7 @@ public function addGroup(string ...$groups): self } // make sure it's a valid group - if (! in_array($group, $configGroups, true)) { + if (! $this->isValidGroup($group)) { throw AuthorizationException::forUnknownGroup($group); } @@ -61,6 +59,18 @@ public function addGroup(string ...$groups): self return $this; } + /** + * @TODO duplicate of UserModel::isValidGroup() + * + * @param non-empty-string $group + */ + private function isValidGroup(string $group): bool + { + $allowedGroups = array_keys(setting('AuthGroups.groups')); + + return (bool) (in_array($group, $allowedGroups, true)); + } + /** * Removes one or more groups from the user. * @@ -96,10 +106,8 @@ public function syncGroups(string ...$groups): self { $this->populateGroups(); - $configGroups = $this->getConfigGroups(); - foreach ($groups as $group) { - if (! in_array($group, $configGroups, true)) { + if (! $this->isValidGroup($group)) { throw AuthorizationException::forUnknownGroup($group); } } @@ -398,14 +406,6 @@ private function saveGroupsOrPermissions(string $type, $model, array $cache): vo } } - /** - * @return list - */ - private function getConfigGroups(): array - { - return array_keys(setting('AuthGroups.groups')); - } - /** * @return list */ diff --git a/src/Models/UserModel.php b/src/Models/UserModel.php index 27020b0b8..f523a64dc 100644 --- a/src/Models/UserModel.php +++ b/src/Models/UserModel.php @@ -153,16 +153,27 @@ private function assignIdentities(array $data, array $identities): array */ public function addToDefaultGroup(User $user): void { - $defaultGroup = setting('AuthGroups.defaultGroup'); - $allowedGroups = array_keys(setting('AuthGroups.groups')); + $defaultGroup = setting('AuthGroups.defaultGroup'); - if (empty($defaultGroup) || ! in_array($defaultGroup, $allowedGroups, true)) { + if (empty($defaultGroup) || ! $this->isValidGroup($defaultGroup)) { throw new InvalidArgumentException(lang('Auth.unknownGroup', [$defaultGroup ?? '--not found--'])); } $user->addGroup($defaultGroup); } + /** + * @TODO duplicate of Authorizable::isValidGroup() + * + * @param non-empty-string $group + */ + private function isValidGroup(string $group): bool + { + $allowedGroups = array_keys(setting('AuthGroups.groups')); + + return (bool) (in_array($group, $allowedGroups, true)); + } + public function fake(Generator &$faker): User { $this->checkReturnType(); From e79bf7870503b9bdfb671fda4d201a6ee5b9dc6d Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 20 Aug 2024 14:38:17 +0900 Subject: [PATCH 3/7] feat: add GroupModel::isValidGroup() --- src/Models/GroupModel.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/Models/GroupModel.php b/src/Models/GroupModel.php index 899931400..0d12ece32 100644 --- a/src/Models/GroupModel.php +++ b/src/Models/GroupModel.php @@ -73,4 +73,14 @@ public function deleteNotIn($userId, $cache): void $this->checkQueryReturn($return); } + + /** + * @param non-empty-string $group Group name + */ + public function isValidGroup(string $group): bool + { + $allowedGroups = array_keys(setting('AuthGroups.groups')); + + return (bool) (in_array($group, $allowedGroups, true)); + } } From 7ae3ff5fdf22ae90e244c942a19429c32e592c96 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 20 Aug 2024 14:38:59 +0900 Subject: [PATCH 4/7] refactor: use GroupModel::isValidGroup() --- src/Authorization/Traits/Authorizable.php | 22 ++++++++-------------- src/Models/UserModel.php | 17 ++++------------- 2 files changed, 12 insertions(+), 27 deletions(-) diff --git a/src/Authorization/Traits/Authorizable.php b/src/Authorization/Traits/Authorizable.php index dc507cf92..3972c0918 100644 --- a/src/Authorization/Traits/Authorizable.php +++ b/src/Authorization/Traits/Authorizable.php @@ -43,8 +43,11 @@ public function addGroup(string ...$groups): self continue; } + /** @var GroupModel $groupModel */ + $groupModel = model(GroupModel::class); + // make sure it's a valid group - if (! $this->isValidGroup($group)) { + if (! $groupModel->isValidGroup($group)) { throw AuthorizationException::forUnknownGroup($group); } @@ -59,18 +62,6 @@ public function addGroup(string ...$groups): self return $this; } - /** - * @TODO duplicate of UserModel::isValidGroup() - * - * @param non-empty-string $group - */ - private function isValidGroup(string $group): bool - { - $allowedGroups = array_keys(setting('AuthGroups.groups')); - - return (bool) (in_array($group, $allowedGroups, true)); - } - /** * Removes one or more groups from the user. * @@ -106,8 +97,11 @@ public function syncGroups(string ...$groups): self { $this->populateGroups(); + /** @var GroupModel $groupModel */ + $groupModel = model(GroupModel::class); + foreach ($groups as $group) { - if (! $this->isValidGroup($group)) { + if (! $groupModel->isValidGroup($group)) { throw AuthorizationException::forUnknownGroup($group); } } diff --git a/src/Models/UserModel.php b/src/Models/UserModel.php index f523a64dc..0fede61ec 100644 --- a/src/Models/UserModel.php +++ b/src/Models/UserModel.php @@ -155,25 +155,16 @@ public function addToDefaultGroup(User $user): void { $defaultGroup = setting('AuthGroups.defaultGroup'); - if (empty($defaultGroup) || ! $this->isValidGroup($defaultGroup)) { + /** @var GroupModel $groupModel */ + $groupModel = model(GroupModel::class); + + if (empty($defaultGroup) || ! $groupModel->isValidGroup($defaultGroup)) { throw new InvalidArgumentException(lang('Auth.unknownGroup', [$defaultGroup ?? '--not found--'])); } $user->addGroup($defaultGroup); } - /** - * @TODO duplicate of Authorizable::isValidGroup() - * - * @param non-empty-string $group - */ - private function isValidGroup(string $group): bool - { - $allowedGroups = array_keys(setting('AuthGroups.groups')); - - return (bool) (in_array($group, $allowedGroups, true)); - } - public function fake(Generator &$faker): User { $this->checkReturnType(); From 641c57387d85683c5c807a0878a7759402dda69d Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 20 Aug 2024 14:58:46 +0900 Subject: [PATCH 5/7] feat: check invalid group name --- src/Commands/User.php | 14 ++++++++++++++ tests/Commands/UserTest.php | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/src/Commands/User.php b/src/Commands/User.php index 9c1a91019..c0977cdf5 100644 --- a/src/Commands/User.php +++ b/src/Commands/User.php @@ -19,6 +19,7 @@ use CodeIgniter\Shield\Config\Auth; use CodeIgniter\Shield\Entities\User as UserEntity; use CodeIgniter\Shield\Exceptions\UserNotFoundException; +use CodeIgniter\Shield\Models\GroupModel; use CodeIgniter\Shield\Models\UserModel; use CodeIgniter\Shield\Validation\ValidationRules; use Config\Services; @@ -305,6 +306,11 @@ private function create(?string $username = null, ?string $email = null, ?string $user = new UserEntity($data); + // Validate the group + if ($group !== null && ! $this->validateGroup($group)) { + throw new CancelException('Invalid group: "' . $group . '"'); + } + if ($username === null) { $userModel->allowEmptyInserts()->save($user); $this->write('New User created', 'green'); @@ -327,6 +333,14 @@ private function create(?string $username = null, ?string $email = null, ?string } } + private function validateGroup(string $group): bool + { + /** @var GroupModel $groupModel */ + $groupModel = model(GroupModel::class); + + return $groupModel->isValidGroup($group); + } + /** * Activate an existing user by username or email * diff --git a/tests/Commands/UserTest.php b/tests/Commands/UserTest.php index 9fdf3e148..b28cd0ae6 100644 --- a/tests/Commands/UserTest.php +++ b/tests/Commands/UserTest.php @@ -134,6 +134,25 @@ public function testCreateWithGroupBeta(): void ]); } + public function testCreateWithInvalidGroup(): void + { + $this->setMockIo([ + 'Secret Passw0rd!', + 'Secret Passw0rd!', + ]); + + command('shield:user create -n user1 -e user1@example.com -g invalid'); + + $this->assertStringContainsString( + 'Invalid group: "invalid"', + $this->io->getFirstOutput() + ); + + $users = model(UserModel::class); + $user = $users->findByCredentials(['email' => 'user1@example.com']); + $this->assertNull($user); + } + public function testCreateNotUniqueName(): void { $user = $this->createUser([ From 0fe12511c3c1edb7df953c45ef54f4c7b54c7dfc Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 20 Aug 2024 15:20:25 +0900 Subject: [PATCH 6/7] refactor: remove unneeded (bool) --- src/Models/GroupModel.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Models/GroupModel.php b/src/Models/GroupModel.php index 0d12ece32..d08c8774d 100644 --- a/src/Models/GroupModel.php +++ b/src/Models/GroupModel.php @@ -81,6 +81,6 @@ public function isValidGroup(string $group): bool { $allowedGroups = array_keys(setting('AuthGroups.groups')); - return (bool) (in_array($group, $allowedGroups, true)); + return in_array($group, $allowedGroups, true); } } From 2e7c54bde663878cfaf9f4755f476ff3fe2f3a0d Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 20 Aug 2024 15:20:58 +0900 Subject: [PATCH 7/7] chore: vendor/bin/phpstan analyze --generate-baseline phpstan-baseline.php --- phpstan-baseline.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/phpstan-baseline.php b/phpstan-baseline.php index 5b2c7aa1b..b6f2501b0 100644 --- a/phpstan-baseline.php +++ b/phpstan-baseline.php @@ -159,6 +159,12 @@ 'count' => 1, 'path' => __DIR__ . '/src/Collectors/Auth.php', ]; +$ignoreErrors[] = [ + // identifier: codeigniter.factoriesClassConstFetch + 'message' => '#^Call to function model with CodeIgniter\\\\Shield\\\\Models\\\\GroupModel\\:\\:class is discouraged\\.$#', + 'count' => 1, + 'path' => __DIR__ . '/src/Commands/User.php', +]; $ignoreErrors[] = [ // identifier: codeigniter.factoriesClassConstFetch 'message' => '#^Call to function model with CodeIgniter\\\\Shield\\\\Models\\\\UserModel\\:\\:class is discouraged\\.$#', @@ -259,7 +265,7 @@ $ignoreErrors[] = [ // identifier: codeigniter.factoriesClassConstFetch 'message' => '#^Call to function model with CodeIgniter\\\\Shield\\\\Models\\\\GroupModel\\:\\:class is discouraged\\.$#', - 'count' => 2, + 'count' => 4, 'path' => __DIR__ . '/src/Entities/User.php', ]; $ignoreErrors[] = [ @@ -330,6 +336,12 @@ 'count' => 1, 'path' => __DIR__ . '/src/Models/UserIdentityModel.php', ]; +$ignoreErrors[] = [ + // identifier: codeigniter.factoriesClassConstFetch + 'message' => '#^Call to function model with CodeIgniter\\\\Shield\\\\Models\\\\GroupModel\\:\\:class is discouraged\\.$#', + 'count' => 1, + 'path' => __DIR__ . '/src/Models/UserModel.php', +]; $ignoreErrors[] = [ // identifier: codeigniter.factoriesClassConstFetch 'message' => '#^Call to function model with CodeIgniter\\\\Shield\\\\Models\\\\UserIdentityModel\\:\\:class is discouraged\\.$#',