Merge pull request #275 from kenjis/fix-usermodel-save

fix: UserModel::save() can't update User's email/password

Author: kenjis

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'    => '[email protected]',
+    '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' => '[email protected]',
-    'password' => 'secret123'
-]);
-
-// Saves the username field
-$users->update($user);
-// Updates the email and password
-$user->saveEmailIdentity();
-```

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,26 +58,28 @@ 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
         if ($user->username === null) {
             $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);
 

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;
     }
 
     /**

src/Exceptions/ValidationException.php

@@ -0,0 +1,9 @@
+<?php
+
+namespace CodeIgniter\Shield\Exceptions;
+
+use RuntimeException;
+
+class ValidationException extends RuntimeException
+{
+}

src/Models/CheckQueryReturnTrait.php

@@ -2,7 +2,7 @@
 
 namespace CodeIgniter\Shield\Models;
 
-use CodeIgniter\Shield\Exceptions\RuntimeException;
+use CodeIgniter\Shield\Exceptions\ValidationException;
 use ReflectionObject;
 use ReflectionProperty;
 
@@ -36,7 +36,7 @@ private function checkValidationError(): void
                 $message .= ' [' . $field . '] ' . $error;
             }
 
-            throw new RuntimeException($message);
+            throw new ValidationException($message);
         }
     }
 

src/Models/UserModel.php

@@ -7,7 +7,7 @@
 use CodeIgniter\Shield\Authentication\Authenticators\Session;
 use CodeIgniter\Shield\Entities\User;
 use CodeIgniter\Shield\Exceptions\InvalidArgumentException;
-use CodeIgniter\Shield\Exceptions\RuntimeException;
+use CodeIgniter\Shield\Exceptions\ValidationException;
 use Faker\Generator;
 
 class UserModel extends Model
@@ -29,13 +29,20 @@ class UserModel extends Model
     ];
     protected $useTimestamps = true;
     protected $afterFind     = ['fetchIdentities'];
+    protected $afterInsert   = ['saveEmailIdentity'];
+    protected $afterUpdate   = ['saveEmailIdentity'];
 
     /**
      * Whether identity records should be included
      * when user records are fetched from the database.
      */
     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
     {
@@ -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;
     }
 }

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.'