diff --git a/docs/quickstart.md b/docs/quickstart.md index f78d7a996..86eeb3f2c 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); ``` @@ -297,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'); @@ -310,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(); -``` diff --git a/src/Controllers/RegisterController.php b/src/Controllers/RegisterController.php index 4bb9994a9..a7a891985 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; /** @@ -57,9 +58,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 @@ -67,16 +71,15 @@ public function registerAction(): RedirectResponse $user->username = null; } - if (! $users->save($user)) { + try { + $users->save($user); + } catch (ValidationException $e) { 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); diff --git a/src/Entities/User.php b/src/Entities/User.php index 6b319f876..1ffd1f8b1 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; @@ -122,7 +123,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. */ @@ -159,7 +160,23 @@ 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; + } + + throw $e; + } + + return true; } /** 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 @@ +fetchIdentities == true * - * Model event callback called `afterFind`. + * Model event callback called by `afterFind`. */ protected function fetchIdentities(array $data): array { @@ -170,6 +177,7 @@ public function findByCredentials(array $credentials): ?User $user = new User($data); $user->email = $email; $user->password_hash = $password_hash; + $user->syncOriginal(); return $user; } @@ -184,48 +192,113 @@ public function activate(User $user): void { $user->active = true; - $return = $this->save($user); - - $this->checkQueryReturn($return); + $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. + * Override the BaseModel's `insert()` method. + * If you pass User object, also inserts Email Identity. * * @param array|User $data * - * @TODO can't change the return type to void. + * @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) { - try { - $result = parent::save($data); + $this->tempUser = $data instanceof User ? $data : null; - if ($result && $data instanceof User) { - /** @var User $user */ - $user = $data->id === null - ? $this->find($this->db->insertID()) - : $data; + $result = parent::insert($data, $returnID); - if (! $user->saveEmailIdentity()) { - throw new RuntimeException('Unable to save email identity.'); - } - } + $this->checkQueryReturn($result); + + return $returnID ? $this->insertID : $result; + } - return $result; + /** + * Override the BaseModel's `update()` method. + * If you pass User object, also updates Email Identity. + * + * @param array|int|string|null $id + * @param array|User $data + * + * @throws ValidationException + */ + public function update($id = null, $data = null): bool + { + $this->tempUser = $data instanceof User ? $data : null; + + try { + /** @throws DataException */ + $result = parent::update($id, $data); } 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? + $this->tempUser->saveEmailIdentity(); + return true; } throw $e; } + + $this->checkQueryReturn($result); + + return true; + } + + /** + * Override the BaseModel's `save()` method. + * If you pass User object, also updates Email Identity. + * + * @param array|User $data + * + * @throws ValidationException + */ + public function save($data): bool + { + $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 + { + // If insert()/update() gets an array data, do nothing. + if ($this->tempUser === null) { + return $data; + } + + // 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(); + $this->tempUser = null; + + return $data; + } + + // Update + $this->tempUser->saveEmailIdentity(); + $this->tempUser = null; + + return $data; } } diff --git a/tests/Unit/LoginModelTest.php b/tests/Unit/LoginModelTest.php index 2292327e1..5c66937aa 100644 --- a/tests/Unit/LoginModelTest.php +++ b/tests/Unit/LoginModelTest.php @@ -2,7 +2,7 @@ namespace Tests\Unit; -use CodeIgniter\Shield\Exceptions\RuntimeException; +use CodeIgniter\Shield\Exceptions\ValidationException; use CodeIgniter\Shield\Models\LoginModel; use CodeIgniter\Test\DatabaseTestTrait; use Tests\Support\TestCase; @@ -24,7 +24,7 @@ private function createLoginModel(): LoginModel public function testRecordLoginAttemptThrowsException(): void { - $this->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.' diff --git a/tests/Unit/UserModelTest.php b/tests/Unit/UserModelTest.php new file mode 100644 index 000000000..e411b6b49 --- /dev/null +++ b/tests/Unit/UserModelTest.php @@ -0,0 +1,201 @@ +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, + ]); + } + + public function testInsertUserObject(): 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, + ]); + } + + 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(); + $user->username = 'foo'; + $user->email = 'foo@bar.com'; + $user->password = 'password'; + $user->active = 0; + + return $user; + } + + public function testSaveUpdateUserObjectWithUserDataToUpdate(): 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 testUpdateUserObjectWithUserDataToUpdate(): 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 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(); + $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', + ]); + } + + public function testUpdateUserObjectWithoutUserDataToUpdate(): 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, + 'secret' => 'bar@bar.com', + ]); + } +}