From 2c9367b42864963f8ca1b00c69b1aa86dff57bf2 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 3 Jul 2022 09:49:49 +0900 Subject: [PATCH 01/17] docs: fix doc comment --- src/Entities/User.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Entities/User.php b/src/Entities/User.php index 6b319f876..88a9b1a7f 100644 --- a/src/Entities/User.php +++ b/src/Entities/User.php @@ -122,7 +122,7 @@ public function getEmailIdentity(): ?UserIdentity } /** - * If $user, $password, or $password_hash have been updated, + * If $email, $password, or $password_hash have been updated, * will update the user's email identity record with the * correct values. */ From 0f029f641bf1992b35d9d7557ca7a984f607cf8b Mon Sep 17 00:00:00 2001 From: kenjis Date: Sun, 3 Jul 2022 09:48:57 +0900 Subject: [PATCH 02/17] fix: UserModel::save() can't update User's email/password --- src/Models/UserModel.php | 16 ++++-- tests/Unit/UserModelTest.php | 96 ++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 tests/Unit/UserModelTest.php diff --git a/src/Models/UserModel.php b/src/Models/UserModel.php index b1859798f..5c4c00a7e 100644 --- a/src/Models/UserModel.php +++ b/src/Models/UserModel.php @@ -204,10 +204,18 @@ public function save($data): bool $result = parent::save($data); if ($result && $data instanceof User) { - /** @var User $user */ - $user = $data->id === null - ? $this->find($this->db->insertID()) - : $data; + if ($data->id === null) { + // Insert + /** @var User $user */ + $user = $this->find($this->db->insertID()); + + $user->email = $data->email ?? null; + $user->password = $data->password ?? ''; + $user->password_hash = $data->password_hash ?? ''; + } else { + // Update + $user = $data; + } if (! $user->saveEmailIdentity()) { throw new RuntimeException('Unable to save email identity.'); diff --git a/tests/Unit/UserModelTest.php b/tests/Unit/UserModelTest.php new file mode 100644 index 000000000..d40345036 --- /dev/null +++ b/tests/Unit/UserModelTest.php @@ -0,0 +1,96 @@ +createUserModel(); + + $user = $this->createNewUser(); + + $users->save($user); + + $user = $users->findByCredentials(['email' => 'foo@bar.com']); + $this->seeInDatabase('auth_identities', [ + 'user_id' => $user->id, + 'secret' => 'foo@bar.com', + ]); + $this->seeInDatabase('users', [ + 'id' => $user->id, + 'active' => 0, + ]); + } + + private function createNewUser(): User + { + $user = new User(); + $user->username = 'foo'; + $user->email = 'foo@bar.com'; + $user->password = 'password'; + $user->active = 0; + + return $user; + } + + public function testSaveUpdateUserWithUserDataToUpdate(): void + { + $users = $this->createUserModel(); + $user = $this->createNewUser(); + $users->save($user); + + $user = $users->findByCredentials(['email' => 'foo@bar.com']); + + $user->username = 'bar'; + $user->email = 'bar@bar.com'; + $user->active = 1; + + $users->save($user); + + $this->seeInDatabase('auth_identities', [ + 'user_id' => $user->id, + 'secret' => 'bar@bar.com', + ]); + $this->seeInDatabase('users', [ + 'id' => $user->id, + 'active' => 1, + ]); + } + + public function testSaveUpdateUserWithNoUserDataToUpdate(): void + { + $users = $this->createUserModel(); + $user = $this->createNewUser(); + $users->save($user); + + $user = $users->findByCredentials(['email' => 'foo@bar.com']); + + $user->email = 'bar@bar.com'; + + $users->save($user); + + $this->seeInDatabase('auth_identities', [ + 'user_id' => $user->id, + 'secret' => 'bar@bar.com', + ]); + } +} From 510f4fba11ea8ff43e3d4f373cb92aca2ef0677e Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 4 Jul 2022 14:23:58 +0900 Subject: [PATCH 03/17] fix: registerAction() so that it does not insert the identity twice --- src/Controllers/RegisterController.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Controllers/RegisterController.php b/src/Controllers/RegisterController.php index 4bb9994a9..675e1c9dc 100644 --- a/src/Controllers/RegisterController.php +++ b/src/Controllers/RegisterController.php @@ -57,9 +57,12 @@ public function registerAction(): RedirectResponse } // Save the user - $allowedPostFields = array_merge(setting('Auth.validFields'), setting('Auth.personalFields')); - $user = $this->getUserEntity(); - + $allowedPostFields = array_merge( + setting('Auth.validFields'), + setting('Auth.personalFields'), + ['password'] + ); + $user = $this->getUserEntity(); $user->fill($this->request->getPost($allowedPostFields)); // Workaround for email only registration/login @@ -71,12 +74,9 @@ public function registerAction(): RedirectResponse return redirect()->back()->withInput()->with('errors', $users->errors()); } - // Get the updated user so we have the ID... + // To get the complete user object with ID, we need to get from the database $user = $users->findById($users->getInsertID()); - // Store the email/password identity for this user. - $user->createEmailIdentity($this->request->getPost(['email', 'password'])); - // Add to default group $users->addToDefaultGroup($user); From d72ac0aa784faaab21a12ac10d2c9123bff01e9a Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 4 Jul 2022 14:25:09 +0900 Subject: [PATCH 04/17] docs: update the docs --- docs/quickstart.md | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/docs/quickstart.md b/docs/quickstart.md index f78d7a996..ffc055f4c 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -265,21 +265,20 @@ Since Shield uses a more complex user setup than many other systems, due to the ### Creating Users -By default, the only values stored in a user is the username. The first step is to create the user record with the username. If you don't have a username, be sure to set the value to `null` anyway, so that it passes CodeIgniter's empty data check. +By default, the only values stored in the users table is the username. The first step is to create the user record with the username. If you don't have a username, be sure to set the value to `null` anyway, so that it passes CodeIgniter's empty data check. ```php $users = model('UserModel'); $user = new User([ - 'username' => 'foo-bar' + 'username' => 'foo-bar', + 'email' => 'foo.bar@example.com', + 'password' => 'secret plain text password', ]); $users->save($user); -// Get the updated user so we have the ID... +// To get the complete user object with ID, we need to get from the database $user = $users->findById($users->getInsertID()); -// Store the email/password identity for this user. -$user->createEmailIdentity($this->request->getPost(['email', 'password'])); - // Add to default group $users->addToDefaultGroup($user); ``` From 2c01462776b64a1d435d9511bc2f977cffb0a9c4 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 4 Jul 2022 14:40:17 +0900 Subject: [PATCH 05/17] fix: add missing syncOriginal() Since we create a User object manually, we need to call it for consistency. --- src/Models/UserModel.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Models/UserModel.php b/src/Models/UserModel.php index 5c4c00a7e..75a54b4ac 100644 --- a/src/Models/UserModel.php +++ b/src/Models/UserModel.php @@ -170,6 +170,7 @@ public function findByCredentials(array $credentials): ?User $user = new User($data); $user->email = $email; $user->password_hash = $password_hash; + $user->syncOriginal(); return $user; } From 7b0145db57790c6b914b1feba6b5fe4a65c06f63 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 4 Jul 2022 15:21:18 +0900 Subject: [PATCH 06/17] fix: can't update User's email/password When updating User's email/password, parent::save($data) may return DataException (no data to update). --- src/Entities/User.php | 17 +++++++++++++- src/Models/UserModel.php | 50 ++++++++++++++++++++++------------------ 2 files changed, 43 insertions(+), 24 deletions(-) diff --git a/src/Entities/User.php b/src/Entities/User.php index 88a9b1a7f..f665ff716 100644 --- a/src/Entities/User.php +++ b/src/Entities/User.php @@ -2,6 +2,7 @@ namespace CodeIgniter\Shield\Entities; +use CodeIgniter\Database\Exceptions\DataException; use CodeIgniter\Entity\Entity; use CodeIgniter\Shield\Authentication\Authenticators\Session; use CodeIgniter\Shield\Authentication\Traits\HasAccessTokens; @@ -159,7 +160,21 @@ public function saveEmailIdentity(): bool /** @var UserIdentityModel $identityModel */ $identityModel = model(UserIdentityModel::class); - return $identityModel->save($identity); + try { + /** @throws DataException */ + $identityModel->save($identity); + } catch (DataException $e) { + // There may be no data to update. + $messages = [ + lang('Database.emptyDataset', ['insert']), + lang('Database.emptyDataset', ['update']), + ]; + if (in_array($e->getMessage(), $messages, true)) { + return true; + } + } + + return true; } /** diff --git a/src/Models/UserModel.php b/src/Models/UserModel.php index 75a54b4ac..bbda0fb4b 100644 --- a/src/Models/UserModel.php +++ b/src/Models/UserModel.php @@ -7,7 +7,6 @@ use CodeIgniter\Shield\Authentication\Authenticators\Session; use CodeIgniter\Shield\Entities\User; use CodeIgniter\Shield\Exceptions\InvalidArgumentException; -use CodeIgniter\Shield\Exceptions\RuntimeException; use Faker\Generator; class UserModel extends Model @@ -202,39 +201,44 @@ public function activate(User $user): void public function save($data): bool { try { + /** @throws DataException */ $result = parent::save($data); - - if ($result && $data instanceof User) { - if ($data->id === null) { - // Insert - /** @var User $user */ - $user = $this->find($this->db->insertID()); - - $user->email = $data->email ?? null; - $user->password = $data->password ?? ''; - $user->password_hash = $data->password_hash ?? ''; - } else { - // Update - $user = $data; - } - - if (! $user->saveEmailIdentity()) { - throw new RuntimeException('Unable to save email identity.'); - } - } - - return $result; } catch (DataException $e) { $messages = [ lang('Database.emptyDataset', ['insert']), lang('Database.emptyDataset', ['update']), ]; if (in_array($e->getMessage(), $messages, true)) { - // @TODO Why true? Shouldn't this workaround be removed? + // Save updated email identity + if ($data instanceof User) { + $user = $data; + + $user->saveEmailIdentity(); + } + return true; } throw $e; } + + if ($result && $data instanceof User) { + if ($data->id === null) { + // Insert + /** @var User $user */ + $user = $this->find($this->db->insertID()); + + $user->email = $data->email ?? null; + $user->password = $data->password ?? ''; + $user->password_hash = $data->password_hash ?? ''; + } else { + // Update + $user = $data; + } + + $user->saveEmailIdentity(); + } + + return true; } } From c657cae97418ca177348837597ada0ac6e45be91 Mon Sep 17 00:00:00 2001 From: kenjis Date: Mon, 4 Jul 2022 15:35:56 +0900 Subject: [PATCH 07/17] fix: add missing `throw $e` --- src/Entities/User.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Entities/User.php b/src/Entities/User.php index f665ff716..1ffd1f8b1 100644 --- a/src/Entities/User.php +++ b/src/Entities/User.php @@ -172,6 +172,8 @@ public function saveEmailIdentity(): bool if (in_array($e->getMessage(), $messages, true)) { return true; } + + throw $e; } return true; From 45cd4c49bf20f750426a53b6d7d5bbcb9b708901 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 5 Jul 2022 11:25:42 +0900 Subject: [PATCH 08/17] refactor: change UserModel::save() to saveWithEmailIdentity() --- docs/concepts.md | 4 +-- docs/quickstart.md | 6 ++--- .../Authenticators/AccessTokens.php | 2 +- src/Authentication/Authenticators/Session.php | 4 +-- src/Controllers/RegisterController.php | 4 +-- src/Models/UserModel.php | 26 +++++-------------- tests/Controllers/ActionsTest.php | 2 +- tests/Unit/UserModelTest.php | 10 +++---- tests/Unit/UserTest.php | 6 ++--- 9 files changed, 25 insertions(+), 39 deletions(-) diff --git a/docs/concepts.md b/docs/concepts.md index 530a0bd67..9fb555382 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -46,8 +46,8 @@ While this has the potential to make the system more complex, the `email` and `p looked up for you when attempting to access them from the User entity. Caution should be used to craft queries that will pull in the `email` field when you need to display it to the user, as you could easily run into some n+1 slow queries otherwise. -When you `save($user)` a `User` instance in the `UserModel`, the email/password identity will automatically be updated. -If no email/password identity exists, you must pass both the email and the password to the User instance prior to calling `save()`. +When you `saveWithEmailIdentity($user)` a `User` instance in the `UserModel`, the email/password identity will automatically be updated. +If no email/password identity exists, you must pass both the email and the password to the User instance prior to calling `saveWithEmailIdentity()`. ## Password Validators diff --git a/docs/quickstart.md b/docs/quickstart.md index ffc055f4c..0eaeb2690 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -274,7 +274,7 @@ $user = new User([ 'email' => 'foo.bar@example.com', 'password' => 'secret plain text password', ]); -$users->save($user); +$users->saveWithEmailIdentity($user); // To get the complete user object with ID, we need to get from the database $user = $users->findById($users->getInsertID()); @@ -296,7 +296,7 @@ NOTE: The User rows use [soft deletes](https://codeigniter.com/user_guide/models ### Editing A User -The `UserModel::save()` method has been modified to ensure that an email or password previously set on the `User` entity will be automatically updated in the correct `UserIdentity` record. +The `UserModel::saveWithEmailIdentity()` method ensures that an email or password previously set on the `User` entity will be automatically updated in the correct `UserIdentity` record. ```php $users = model('UserModel'); @@ -307,7 +307,7 @@ $user->fill([ 'email' => 'joe.smith@example.com', 'password' => 'secret123' ]); -$users->save($user); +$users->saveWithEmailIdentity($user); ``` If you prefer to use the `update()` method then you will have to update the user's appropriate UserIdentity manually. diff --git a/src/Authentication/Authenticators/AccessTokens.php b/src/Authentication/Authenticators/AccessTokens.php index 3bf64f587..a370e250e 100644 --- a/src/Authentication/Authenticators/AccessTokens.php +++ b/src/Authentication/Authenticators/AccessTokens.php @@ -232,6 +232,6 @@ public function recordActiveDate(): void $this->user->last_active = Time::now(); - $this->provider->save($this->user); + $this->provider->saveWithEmailIdentity($this->user); } } diff --git a/src/Authentication/Authenticators/Session.php b/src/Authentication/Authenticators/Session.php index 5f91f4c3e..3cbb81db5 100644 --- a/src/Authentication/Authenticators/Session.php +++ b/src/Authentication/Authenticators/Session.php @@ -310,7 +310,7 @@ public function check(array $credentials): Result // logged in. if ($passwords->needsRehash($user->password_hash)) { $user->password_hash = $passwords->hash($givenPassword); - $this->provider->save($user); + $this->provider->saveWithEmailIdentity($user); } return new Result([ @@ -803,7 +803,7 @@ public function recordActiveDate(): void $this->user->last_active = Time::now(); - $this->provider->save($this->user); + $this->provider->saveWithEmailIdentity($this->user); } /** diff --git a/src/Controllers/RegisterController.php b/src/Controllers/RegisterController.php index 675e1c9dc..35d37d8b1 100644 --- a/src/Controllers/RegisterController.php +++ b/src/Controllers/RegisterController.php @@ -70,9 +70,7 @@ public function registerAction(): RedirectResponse $user->username = null; } - if (! $users->save($user)) { - return redirect()->back()->withInput()->with('errors', $users->errors()); - } + $users->saveWithEmailIdentity($user); // To get the complete user object with ID, we need to get from the database $user = $users->findById($users->getInsertID()); diff --git a/src/Models/UserModel.php b/src/Models/UserModel.php index bbda0fb4b..be23cf287 100644 --- a/src/Models/UserModel.php +++ b/src/Models/UserModel.php @@ -184,21 +184,14 @@ public function activate(User $user): void { $user->active = true; - $return = $this->save($user); - - $this->checkQueryReturn($return); + $this->saveWithEmailIdentity($user); } /** - * Override the BaseModel's `save()` method to allow - * updating of user email, password, or password_hash - * fields if they've been modified. - * - * @param array|User $data - * - * @TODO can't change the return type to void. + * Save User and its Email Identity (email, password, or password_hash fields) + * if they've been modified. */ - public function save($data): bool + public function saveWithEmailIdentity(User $data): void { try { /** @throws DataException */ @@ -210,13 +203,10 @@ public function save($data): bool ]; if (in_array($e->getMessage(), $messages, true)) { // Save updated email identity - if ($data instanceof User) { - $user = $data; - - $user->saveEmailIdentity(); - } + $user = $data; + $user->saveEmailIdentity(); - return true; + return; } throw $e; @@ -238,7 +228,5 @@ public function save($data): bool $user->saveEmailIdentity(); } - - return true; } } diff --git a/tests/Controllers/ActionsTest.php b/tests/Controllers/ActionsTest.php index 002ce6d25..9a417c378 100644 --- a/tests/Controllers/ActionsTest.php +++ b/tests/Controllers/ActionsTest.php @@ -247,7 +247,7 @@ public function testEmailActivateVerify(): void $this->user->active = false; $model = auth()->getProvider(); - $model->save($this->user); + $model->saveWithEmailIdentity($this->user); $result = $this->actingAs($this->user, true) ->withSession($this->getSessionUserInfo(EmailActivator::class)) diff --git a/tests/Unit/UserModelTest.php b/tests/Unit/UserModelTest.php index d40345036..6be037ad4 100644 --- a/tests/Unit/UserModelTest.php +++ b/tests/Unit/UserModelTest.php @@ -28,7 +28,7 @@ public function testSaveInsertUser(): void $user = $this->createNewUser(); - $users->save($user); + $users->saveWithEmailIdentity($user); $user = $users->findByCredentials(['email' => 'foo@bar.com']); $this->seeInDatabase('auth_identities', [ @@ -56,7 +56,7 @@ public function testSaveUpdateUserWithUserDataToUpdate(): void { $users = $this->createUserModel(); $user = $this->createNewUser(); - $users->save($user); + $users->saveWithEmailIdentity($user); $user = $users->findByCredentials(['email' => 'foo@bar.com']); @@ -64,7 +64,7 @@ public function testSaveUpdateUserWithUserDataToUpdate(): void $user->email = 'bar@bar.com'; $user->active = 1; - $users->save($user); + $users->saveWithEmailIdentity($user); $this->seeInDatabase('auth_identities', [ 'user_id' => $user->id, @@ -80,13 +80,13 @@ public function testSaveUpdateUserWithNoUserDataToUpdate(): void { $users = $this->createUserModel(); $user = $this->createNewUser(); - $users->save($user); + $users->saveWithEmailIdentity($user); $user = $users->findByCredentials(['email' => 'foo@bar.com']); $user->email = 'bar@bar.com'; - $users->save($user); + $users->saveWithEmailIdentity($user); $this->seeInDatabase('auth_identities', [ 'user_id' => $user->id, diff --git a/tests/Unit/UserTest.php b/tests/Unit/UserTest.php index 185111aa3..b86a716b2 100644 --- a/tests/Unit/UserTest.php +++ b/tests/Unit/UserTest.php @@ -112,7 +112,7 @@ public function testUpdateEmail(): void $this->user->active = 0; $users = model(UserModel::class); - $users->save($this->user); + $users->saveWithEmailIdentity($this->user); $user = $users->find($this->user->id); @@ -134,7 +134,7 @@ public function testUpdatePassword(): void $this->user->active = 0; $users = model(UserModel::class); - $users->save($this->user); + $users->saveWithEmailIdentity($this->user); $user = $users->find($this->user->id); @@ -153,7 +153,7 @@ public function testUpdatePasswordHash(): void $this->user->active = 0; $users = model(UserModel::class); - $users->save($this->user); + $users->saveWithEmailIdentity($this->user); $user = $users->find($this->user->id); From a3f7e109ddd6a8e5733be94f5c9ac7f97c71100b Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 5 Jul 2022 14:57:57 +0900 Subject: [PATCH 09/17] refactor: remove unnecessary if condition --- src/Models/UserModel.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Models/UserModel.php b/src/Models/UserModel.php index be23cf287..bdf66b511 100644 --- a/src/Models/UserModel.php +++ b/src/Models/UserModel.php @@ -212,7 +212,7 @@ public function saveWithEmailIdentity(User $data): void throw $e; } - if ($result && $data instanceof User) { + if ($result) { if ($data->id === null) { // Insert /** @var User $user */ From e9604a94e777ddbca9bc7b93c85cceb78c3760fb Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 5 Jul 2022 15:32:25 +0900 Subject: [PATCH 10/17] fix: add missing checkQueryReturn() --- src/Models/UserModel.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Models/UserModel.php b/src/Models/UserModel.php index bdf66b511..300852275 100644 --- a/src/Models/UserModel.php +++ b/src/Models/UserModel.php @@ -228,5 +228,7 @@ public function saveWithEmailIdentity(User $data): void $user->saveEmailIdentity(); } + + $this->checkQueryReturn($result); } } From 95953f48b755356c13c7af592a59e1d7d7c41416 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 6 Jul 2022 11:07:03 +0900 Subject: [PATCH 11/17] refactor: add ValidationException and use it --- src/Exceptions/ValidationException.php | 9 +++++++++ src/Models/CheckQueryReturnTrait.php | 4 ++-- tests/Unit/LoginModelTest.php | 4 ++-- 3 files changed, 13 insertions(+), 4 deletions(-) create mode 100644 src/Exceptions/ValidationException.php diff --git a/src/Exceptions/ValidationException.php b/src/Exceptions/ValidationException.php new file mode 100644 index 000000000..2ca44e727 --- /dev/null +++ b/src/Exceptions/ValidationException.php @@ -0,0 +1,9 @@ +expectException(RuntimeException::class); + $this->expectException(ValidationException::class); $this->expectExceptionMessage( 'Validation error: [ip_address] The ip_address field is required.' . ' [id_type] The id_type field is required.' From f17355e8c347bd2d49213b385198ab6ccc8450aa Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 6 Jul 2022 11:10:19 +0900 Subject: [PATCH 12/17] fix: catch ValidationException --- src/Controllers/RegisterController.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Controllers/RegisterController.php b/src/Controllers/RegisterController.php index 35d37d8b1..9e718eaec 100644 --- a/src/Controllers/RegisterController.php +++ b/src/Controllers/RegisterController.php @@ -7,6 +7,7 @@ use CodeIgniter\HTTP\RedirectResponse; use CodeIgniter\Shield\Authentication\Authenticators\Session; use CodeIgniter\Shield\Entities\User; +use CodeIgniter\Shield\Exceptions\ValidationException; use CodeIgniter\Shield\Models\UserModel; /** @@ -70,7 +71,11 @@ public function registerAction(): RedirectResponse $user->username = null; } - $users->saveWithEmailIdentity($user); + try { + $users->saveWithEmailIdentity($user); + } catch (ValidationException $e) { + return redirect()->back()->withInput()->with('errors', $users->errors()); + } // To get the complete user object with ID, we need to get from the database $user = $users->findById($users->getInsertID()); From 9487136a640c7b2e3b6a789f3f36f6c9e1b742a8 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 7 Jul 2022 15:21:03 +0900 Subject: [PATCH 13/17] feat: UserModel::save() can save Email Identity again --- docs/concepts.md | 4 ++-- docs/quickstart.md | 6 +++--- .../Authenticators/AccessTokens.php | 2 +- src/Authentication/Authenticators/Session.php | 4 ++-- src/Controllers/RegisterController.php | 2 +- src/Models/UserModel.php | 21 +++++++++++++++++++ tests/Unit/UserModelTest.php | 8 +++---- 7 files changed, 34 insertions(+), 13 deletions(-) diff --git a/docs/concepts.md b/docs/concepts.md index 9fb555382..530a0bd67 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -46,8 +46,8 @@ While this has the potential to make the system more complex, the `email` and `p looked up for you when attempting to access them from the User entity. Caution should be used to craft queries that will pull in the `email` field when you need to display it to the user, as you could easily run into some n+1 slow queries otherwise. -When you `saveWithEmailIdentity($user)` a `User` instance in the `UserModel`, the email/password identity will automatically be updated. -If no email/password identity exists, you must pass both the email and the password to the User instance prior to calling `saveWithEmailIdentity()`. +When you `save($user)` a `User` instance in the `UserModel`, the email/password identity will automatically be updated. +If no email/password identity exists, you must pass both the email and the password to the User instance prior to calling `save()`. ## Password Validators diff --git a/docs/quickstart.md b/docs/quickstart.md index 0eaeb2690..ffc055f4c 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -274,7 +274,7 @@ $user = new User([ 'email' => 'foo.bar@example.com', 'password' => 'secret plain text password', ]); -$users->saveWithEmailIdentity($user); +$users->save($user); // To get the complete user object with ID, we need to get from the database $user = $users->findById($users->getInsertID()); @@ -296,7 +296,7 @@ NOTE: The User rows use [soft deletes](https://codeigniter.com/user_guide/models ### Editing A User -The `UserModel::saveWithEmailIdentity()` method ensures that an email or password previously set on the `User` entity will be automatically updated in the correct `UserIdentity` record. +The `UserModel::save()` method has been modified to ensure that an email or password previously set on the `User` entity will be automatically updated in the correct `UserIdentity` record. ```php $users = model('UserModel'); @@ -307,7 +307,7 @@ $user->fill([ 'email' => 'joe.smith@example.com', 'password' => 'secret123' ]); -$users->saveWithEmailIdentity($user); +$users->save($user); ``` If you prefer to use the `update()` method then you will have to update the user's appropriate UserIdentity manually. diff --git a/src/Authentication/Authenticators/AccessTokens.php b/src/Authentication/Authenticators/AccessTokens.php index a370e250e..3bf64f587 100644 --- a/src/Authentication/Authenticators/AccessTokens.php +++ b/src/Authentication/Authenticators/AccessTokens.php @@ -232,6 +232,6 @@ public function recordActiveDate(): void $this->user->last_active = Time::now(); - $this->provider->saveWithEmailIdentity($this->user); + $this->provider->save($this->user); } } diff --git a/src/Authentication/Authenticators/Session.php b/src/Authentication/Authenticators/Session.php index 3cbb81db5..5f91f4c3e 100644 --- a/src/Authentication/Authenticators/Session.php +++ b/src/Authentication/Authenticators/Session.php @@ -310,7 +310,7 @@ public function check(array $credentials): Result // logged in. if ($passwords->needsRehash($user->password_hash)) { $user->password_hash = $passwords->hash($givenPassword); - $this->provider->saveWithEmailIdentity($user); + $this->provider->save($user); } return new Result([ @@ -803,7 +803,7 @@ public function recordActiveDate(): void $this->user->last_active = Time::now(); - $this->provider->saveWithEmailIdentity($this->user); + $this->provider->save($this->user); } /** diff --git a/src/Controllers/RegisterController.php b/src/Controllers/RegisterController.php index 9e718eaec..a7a891985 100644 --- a/src/Controllers/RegisterController.php +++ b/src/Controllers/RegisterController.php @@ -72,7 +72,7 @@ public function registerAction(): RedirectResponse } try { - $users->saveWithEmailIdentity($user); + $users->save($user); } catch (ValidationException $e) { return redirect()->back()->withInput()->with('errors', $users->errors()); } diff --git a/src/Models/UserModel.php b/src/Models/UserModel.php index 300852275..625bb5226 100644 --- a/src/Models/UserModel.php +++ b/src/Models/UserModel.php @@ -7,6 +7,7 @@ use CodeIgniter\Shield\Authentication\Authenticators\Session; use CodeIgniter\Shield\Entities\User; use CodeIgniter\Shield\Exceptions\InvalidArgumentException; +use CodeIgniter\Shield\Exceptions\ValidationException; use Faker\Generator; class UserModel extends Model @@ -187,9 +188,29 @@ public function activate(User $user): void $this->saveWithEmailIdentity($user); } + /** + * Override the BaseModel's `save()` method to allow + * updating of user email, password, or password_hash fields + * if they've been modified. + * + * @param User $data + * + * @throws ValidationException + */ + public function save($data): bool + { + assert($data instanceof User); + + $this->saveWithEmailIdentity($data); + + return true; + } + /** * Save User and its Email Identity (email, password, or password_hash fields) * if they've been modified. + * + * @throws ValidationException */ public function saveWithEmailIdentity(User $data): void { diff --git a/tests/Unit/UserModelTest.php b/tests/Unit/UserModelTest.php index 6be037ad4..cf009cf14 100644 --- a/tests/Unit/UserModelTest.php +++ b/tests/Unit/UserModelTest.php @@ -28,7 +28,7 @@ public function testSaveInsertUser(): void $user = $this->createNewUser(); - $users->saveWithEmailIdentity($user); + $users->save($user); $user = $users->findByCredentials(['email' => 'foo@bar.com']); $this->seeInDatabase('auth_identities', [ @@ -56,7 +56,7 @@ public function testSaveUpdateUserWithUserDataToUpdate(): void { $users = $this->createUserModel(); $user = $this->createNewUser(); - $users->saveWithEmailIdentity($user); + $users->save($user); $user = $users->findByCredentials(['email' => 'foo@bar.com']); @@ -64,7 +64,7 @@ public function testSaveUpdateUserWithUserDataToUpdate(): void $user->email = 'bar@bar.com'; $user->active = 1; - $users->saveWithEmailIdentity($user); + $users->save($user); $this->seeInDatabase('auth_identities', [ 'user_id' => $user->id, @@ -80,7 +80,7 @@ public function testSaveUpdateUserWithNoUserDataToUpdate(): void { $users = $this->createUserModel(); $user = $this->createNewUser(); - $users->saveWithEmailIdentity($user); + $users->save($user); $user = $users->findByCredentials(['email' => 'foo@bar.com']); From 51bb61bc704b020c4465454728d0cb65ba314fcf Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 12 Jul 2022 15:50:32 +0900 Subject: [PATCH 14/17] feat: insert(), update(), save() can save Email Identity with Model Events Remove saveWithEmailIdentity(). --- src/Models/UserModel.php | 107 +++++++++++++++++++++--------- tests/Controllers/ActionsTest.php | 2 +- tests/Unit/UserModelTest.php | 63 +++++++++++++++++- tests/Unit/UserTest.php | 6 +- 4 files changed, 141 insertions(+), 37 deletions(-) diff --git a/src/Models/UserModel.php b/src/Models/UserModel.php index 625bb5226..07d2b80e6 100644 --- a/src/Models/UserModel.php +++ b/src/Models/UserModel.php @@ -29,6 +29,8 @@ class UserModel extends Model ]; protected $useTimestamps = true; protected $afterFind = ['fetchIdentities']; + protected $afterInsert = ['saveEmailIdentity']; + protected $afterUpdate = ['saveEmailIdentity']; /** * Whether identity records should be included @@ -36,6 +38,11 @@ class UserModel extends Model */ protected bool $fetchIdentities = false; + /** + * Save the User for afterInsert and afterUpdate + */ + protected ?User $tempUser = null; + /** * Mark the next find* query to include identities * @@ -53,7 +60,7 @@ public function withIdentities(): self * returned from a find* method. Called * automatically when $this->fetchIdentities == true * - * Model event callback called `afterFind`. + * Model event callback called by `afterFind`. */ protected function fetchIdentities(array $data): array { @@ -185,71 +192,107 @@ public function activate(User $user): void { $user->active = true; - $this->saveWithEmailIdentity($user); + $this->save($user); } /** - * Override the BaseModel's `save()` method to allow - * updating of user email, password, or password_hash fields - * if they've been modified. - * * @param User $data * * @throws ValidationException + * + * @retrun true|int|string Insert ID if $returnID is true */ - public function save($data): bool + public function insert($data = null, bool $returnID = true) { assert($data instanceof User); - $this->saveWithEmailIdentity($data); + $this->tempUser = $data; - return true; + $result = parent::insert($data, $returnID); + + $this->checkQueryReturn($result); + + return $returnID ? $this->insertID : $result; } /** - * Save User and its Email Identity (email, password, or password_hash fields) - * if they've been modified. + * @param array|int|string|null $id + * @param User $data * * @throws ValidationException */ - public function saveWithEmailIdentity(User $data): void + public function update($id = null, $data = null): bool { + assert($data instanceof User); + + $this->tempUser = $data; + try { /** @throws DataException */ - $result = parent::save($data); + $result = parent::update($id, $data); } catch (DataException $e) { $messages = [ - lang('Database.emptyDataset', ['insert']), lang('Database.emptyDataset', ['update']), ]; + if (in_array($e->getMessage(), $messages, true)) { - // Save updated email identity - $user = $data; - $user->saveEmailIdentity(); + $this->tempUser->saveEmailIdentity(); - return; + return true; } throw $e; } - if ($result) { - if ($data->id === null) { - // Insert - /** @var User $user */ - $user = $this->find($this->db->insertID()); - - $user->email = $data->email ?? null; - $user->password = $data->password ?? ''; - $user->password_hash = $data->password_hash ?? ''; - } else { - // Update - $user = $data; - } + $this->checkQueryReturn($result); + + return true; + } + + /** + * Override the BaseModel's `save()` method to allow + * updating of user email, password, or password_hash fields + * if they've been modified. + * + * @param User $data + * + * @throws ValidationException + */ + public function save($data): bool + { + assert($data instanceof User); + + $result = parent::save($data); + + $this->checkQueryReturn($result); + + return true; + } + + /** + * Save Email Identity + * + * Model event callback called by `afterInsert` and `afterUpdate`. + */ + protected function saveEmailIdentity(array $data): array + { + // Insert + if ($this->tempUser->id === null) { + /** @var User $user */ + $user = $this->find($this->db->insertID()); + + $user->email = $this->tempUser->email ?? ''; + $user->password = $this->tempUser->password ?? ''; + $user->password_hash = $this->tempUser->password_hash ?? ''; $user->saveEmailIdentity(); + + return $data; } - $this->checkQueryReturn($result); + // Update + $this->tempUser->saveEmailIdentity(); + + return $data; } } diff --git a/tests/Controllers/ActionsTest.php b/tests/Controllers/ActionsTest.php index 9a417c378..002ce6d25 100644 --- a/tests/Controllers/ActionsTest.php +++ b/tests/Controllers/ActionsTest.php @@ -247,7 +247,7 @@ public function testEmailActivateVerify(): void $this->user->active = false; $model = auth()->getProvider(); - $model->saveWithEmailIdentity($this->user); + $model->save($this->user); $result = $this->actingAs($this->user, true) ->withSession($this->getSessionUserInfo(EmailActivator::class)) diff --git a/tests/Unit/UserModelTest.php b/tests/Unit/UserModelTest.php index cf009cf14..ebf0c15b2 100644 --- a/tests/Unit/UserModelTest.php +++ b/tests/Unit/UserModelTest.php @@ -41,6 +41,25 @@ public function testSaveInsertUser(): void ]); } + public function testInsertUser(): void + { + $users = $this->createUserModel(); + + $user = $this->createNewUser(); + + $users->insert($user); + + $user = $users->findByCredentials(['email' => 'foo@bar.com']); + $this->seeInDatabase('auth_identities', [ + 'user_id' => $user->id, + 'secret' => 'foo@bar.com', + ]); + $this->seeInDatabase('users', [ + 'id' => $user->id, + 'active' => 0, + ]); + } + private function createNewUser(): User { $user = new User(); @@ -76,6 +95,30 @@ public function testSaveUpdateUserWithUserDataToUpdate(): void ]); } + public function testUpdateUserWithUserDataToUpdate(): void + { + $users = $this->createUserModel(); + $user = $this->createNewUser(); + $users->save($user); + + $user = $users->findByCredentials(['email' => 'foo@bar.com']); + + $user->username = 'bar'; + $user->email = 'bar@bar.com'; + $user->active = 1; + + $users->update(null, $user); + + $this->seeInDatabase('auth_identities', [ + 'user_id' => $user->id, + 'secret' => 'bar@bar.com', + ]); + $this->seeInDatabase('users', [ + 'id' => $user->id, + 'active' => 1, + ]); + } + public function testSaveUpdateUserWithNoUserDataToUpdate(): void { $users = $this->createUserModel(); @@ -86,7 +129,25 @@ public function testSaveUpdateUserWithNoUserDataToUpdate(): void $user->email = 'bar@bar.com'; - $users->saveWithEmailIdentity($user); + $users->save($user); + + $this->seeInDatabase('auth_identities', [ + 'user_id' => $user->id, + 'secret' => 'bar@bar.com', + ]); + } + + public function testUpdateUserWithNoUserDataToUpdate(): void + { + $users = $this->createUserModel(); + $user = $this->createNewUser(); + $users->save($user); + + $user = $users->findByCredentials(['email' => 'foo@bar.com']); + + $user->email = 'bar@bar.com'; + + $users->update(null, $user); $this->seeInDatabase('auth_identities', [ 'user_id' => $user->id, diff --git a/tests/Unit/UserTest.php b/tests/Unit/UserTest.php index b86a716b2..185111aa3 100644 --- a/tests/Unit/UserTest.php +++ b/tests/Unit/UserTest.php @@ -112,7 +112,7 @@ public function testUpdateEmail(): void $this->user->active = 0; $users = model(UserModel::class); - $users->saveWithEmailIdentity($this->user); + $users->save($this->user); $user = $users->find($this->user->id); @@ -134,7 +134,7 @@ public function testUpdatePassword(): void $this->user->active = 0; $users = model(UserModel::class); - $users->saveWithEmailIdentity($this->user); + $users->save($this->user); $user = $users->find($this->user->id); @@ -153,7 +153,7 @@ public function testUpdatePasswordHash(): void $this->user->active = 0; $users = model(UserModel::class); - $users->saveWithEmailIdentity($this->user); + $users->save($this->user); $user = $users->find($this->user->id); From af13b4c22fc902c13e7bad476a92c5e2a7125570 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 12 Jul 2022 16:00:29 +0900 Subject: [PATCH 15/17] docs: update about UserModel::insert() and update() --- docs/quickstart.md | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/docs/quickstart.md b/docs/quickstart.md index ffc055f4c..86eeb3f2c 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -296,7 +296,7 @@ NOTE: The User rows use [soft deletes](https://codeigniter.com/user_guide/models ### Editing A User -The `UserModel::save()` method has been modified to ensure that an email or password previously set on the `User` entity will be automatically updated in the correct `UserIdentity` record. +The `UserModel::save()`, `update()` and `insert()` methods have been modified to ensure that an email or password previously set on the `User` entity will be automatically updated in the correct `UserIdentity` record. ```php $users = model('UserModel'); @@ -309,21 +309,3 @@ $user->fill([ ]); $users->save($user); ``` - -If you prefer to use the `update()` method then you will have to update the user's appropriate UserIdentity manually. - -```php -$users = model('UserModel'); -$user = $users->findById(123); - -$user->fill([ - 'username' => 'JoeSmith111', - 'email' => 'joe.smith@example.com', - 'password' => 'secret123' -]); - -// Saves the username field -$users->update($user); -// Updates the email and password -$user->saveEmailIdentity(); -``` From 88680067809b55378dda3867fa4223fdca115ef9 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 14 Jul 2022 09:52:41 +0900 Subject: [PATCH 16/17] feat: when you pass an array user data to insert()/update(), Model events do not save Email Identity --- src/Models/UserModel.php | 32 +++++++++++---------- tests/Unit/UserModelTest.php | 54 ++++++++++++++++++++++++++++++++---- 2 files changed, 67 insertions(+), 19 deletions(-) diff --git a/src/Models/UserModel.php b/src/Models/UserModel.php index 07d2b80e6..1663ccc7d 100644 --- a/src/Models/UserModel.php +++ b/src/Models/UserModel.php @@ -196,7 +196,10 @@ public function activate(User $user): void } /** - * @param User $data + * Override the BaseModel's `insert()` method. + * If you pass User object, also inserts Email Identity. + * + * @param array|User $data * * @throws ValidationException * @@ -204,9 +207,7 @@ public function activate(User $user): void */ public function insert($data = null, bool $returnID = true) { - assert($data instanceof User); - - $this->tempUser = $data; + $this->tempUser = $data instanceof User ? $data : null; $result = parent::insert($data, $returnID); @@ -216,16 +217,17 @@ public function insert($data = null, bool $returnID = true) } /** + * Override the BaseModel's `update()` method. + * If you pass User object, also updates Email Identity. + * * @param array|int|string|null $id - * @param User $data + * @param array|User $data * * @throws ValidationException */ public function update($id = null, $data = null): bool { - assert($data instanceof User); - - $this->tempUser = $data; + $this->tempUser = $data instanceof User ? $data : null; try { /** @throws DataException */ @@ -250,18 +252,15 @@ public function update($id = null, $data = null): bool } /** - * Override the BaseModel's `save()` method to allow - * updating of user email, password, or password_hash fields - * if they've been modified. + * Override the BaseModel's `save()` method. + * If you pass User object, also updates Email Identity. * - * @param User $data + * @param array|User $data * * @throws ValidationException */ public function save($data): bool { - assert($data instanceof User); - $result = parent::save($data); $this->checkQueryReturn($result); @@ -276,6 +275,11 @@ public function save($data): bool */ protected function saveEmailIdentity(array $data): array { + // If insert()/update() gets an array data, do nothing. + if ($this->tempUser === null) { + return $data; + } + // Insert if ($this->tempUser->id === null) { /** @var User $user */ diff --git a/tests/Unit/UserModelTest.php b/tests/Unit/UserModelTest.php index ebf0c15b2..e411b6b49 100644 --- a/tests/Unit/UserModelTest.php +++ b/tests/Unit/UserModelTest.php @@ -41,7 +41,7 @@ public function testSaveInsertUser(): void ]); } - public function testInsertUser(): void + public function testInsertUserObject(): void { $users = $this->createUserModel(); @@ -60,6 +60,25 @@ public function testInsertUser(): void ]); } + public function testInsertUserArray(): void + { + $users = $this->createUserModel(); + + $user = $this->createNewUser(); + + $userArray = $user->toArray(); + $id = $users->insert($userArray); + + $this->dontSeeInDatabase('auth_identities', [ + 'user_id' => $id, + 'secret' => 'foo@bar.com', + ]); + $this->seeInDatabase('users', [ + 'id' => $id, + 'active' => 0, + ]); + } + private function createNewUser(): User { $user = new User(); @@ -71,7 +90,7 @@ private function createNewUser(): User return $user; } - public function testSaveUpdateUserWithUserDataToUpdate(): void + public function testSaveUpdateUserObjectWithUserDataToUpdate(): void { $users = $this->createUserModel(); $user = $this->createNewUser(); @@ -95,7 +114,7 @@ public function testSaveUpdateUserWithUserDataToUpdate(): void ]); } - public function testUpdateUserWithUserDataToUpdate(): void + public function testUpdateUserObjectWithUserDataToUpdate(): void { $users = $this->createUserModel(); $user = $this->createNewUser(); @@ -119,7 +138,32 @@ public function testUpdateUserWithUserDataToUpdate(): void ]); } - public function testSaveUpdateUserWithNoUserDataToUpdate(): void + public function testUpdateUserArrayWithUserDataToUpdate(): void + { + $users = $this->createUserModel(); + $user = $this->createNewUser(); + $users->save($user); + + $user = $users->findByCredentials(['email' => 'foo@bar.com']); + + $user->username = 'bar'; + $user->email = 'bar@bar.com'; + $user->active = 1; + + $userArray = $user->toArray(); + $users->update(null, $userArray); + + $this->dontSeeInDatabase('auth_identities', [ + 'user_id' => $user->id, + 'secret' => 'bar@bar.com', + ]); + $this->seeInDatabase('users', [ + 'id' => $user->id, + 'active' => 1, + ]); + } + + public function testSaveUpdateUserObjectWithoutUserDataToUpdate(): void { $users = $this->createUserModel(); $user = $this->createNewUser(); @@ -137,7 +181,7 @@ public function testSaveUpdateUserWithNoUserDataToUpdate(): void ]); } - public function testUpdateUserWithNoUserDataToUpdate(): void + public function testUpdateUserObjectWithoutUserDataToUpdate(): void { $users = $this->createUserModel(); $user = $this->createNewUser(); From 1ecdbdd9a474ad2d7e2797d70f545819dcf5badd Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 15 Jul 2022 17:31:54 +0900 Subject: [PATCH 17/17] fix: reset $this->tempUser after save Email Identity --- src/Models/UserModel.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Models/UserModel.php b/src/Models/UserModel.php index 1663ccc7d..e60d3e40b 100644 --- a/src/Models/UserModel.php +++ b/src/Models/UserModel.php @@ -290,12 +290,14 @@ protected function saveEmailIdentity(array $data): array $user->password_hash = $this->tempUser->password_hash ?? ''; $user->saveEmailIdentity(); + $this->tempUser = null; return $data; } // Update $this->tempUser->saveEmailIdentity(); + $this->tempUser = null; return $data; }