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

kenjis · 2022-07-03T00:52:04Z

Fixes #254
Fixes #253

UserModel::save(), update() and insert() save Email Identity with Model Events
fix UserModel::findByCredentials()
fix RegisterController::registerAction()
fix User::saveEmailIdentity()
add ValidationException
update docs/quickstart.md

MGatner

If this works then let's go with it but I still have plans to add the original method parameters to the event data in the framework, which would make this much easier.

MGatner · 2022-07-03T11:38:37Z

Sorry, I didn't mean to hit "approve" - I'm good with the approach but obviously will need to pass tests 😂

datamweb · 2022-07-03T13:03:12Z

Sorry, I didn't mean to hit "approve" - I'm good with the approach but obviously will need to pass tests 😂

@MGatner this PR is in position Draft. But you are in a hurry to improve Shield. This is great.

kenjis · 2022-07-03T21:10:37Z

I removed the approval.

kenjis · 2022-07-03T21:12:20Z

If this works then let's go with it but I still have plans to add the original method parameters to the event data in the framework, which would make this much easier.

The enhancement is good, but we need to wait for v4.3. So I think this PR is needed.

kenjis · 2022-07-04T06:43:52Z

Okay, I think this is ready for reviews.

kenjis · 2022-07-04T06:53:52Z

I think UserModel::save() should be renamed.

It is confusing only save() is overridden and can save email/password. insert()/update() does not work.
Can't change the method signature.

MGatner · 2022-07-04T10:56:48Z

@kenjis I agree. I think the ultimate solution is to modify CodeIgniter\BaseModel::update() (and probably insert()) to preserve the original $data parameter and pass that as an additional key into the beforeInsert and afterInsert methods. That would give Shield access to the User Entity without needing a separate database query and it wouldn't need to override any model methods.
Since that will require a new framework release I like your solution for the meantime... something like UserModel::saveWithIdentities(User $user) right?

kenjis · 2022-07-05T02:27:13Z

something like UserModel::saveWithIdentities(User $user) right?

Yes. I added a commit: 39aaf14

src/Models/UserModel.php

src/Entities/User.php

src/Controllers/RegisterController.php

lonnieezell · 2022-07-06T03:53:24Z

src/Models/UserModel.php

     */
-    public function save($data): bool
+    public function saveWithEmailIdentity(User $data): void


Did we need to rename this for a specific reason? If so, or the team majority says we should then ok, but I'm in favor of keeping it as an override of Model::save.

I understand this is a more specific naming, but the whole purpose of the override was to make it as seamless as possible for the developer so they didn't have to think too much about it. This forces them to remember a new method for that single model, which is inconvenient and, as far as I can tell, unnecessary.

Reading back through I see the discussion between you two so I'm overruled here. Just going on the record to say I disagree :)

👹 Contention noted!

I think this does two good things for us:

It lets us still use save() without messing with identities.

It allows us to force the User type parameter which makes this method much easier to handle.

Also just to say it again: this should just be a temporary measure until we get better Model event parameters in place.

I overrode save() again.

After all, if save() is going to be used in the future, it is better for users to use save() now.

It is confusing only save() can save Email Identity, but users do not need to use insert() or update().

Alternative idea...

The whole reason we override this method is that afterInsert and afterUpdate receive the data as an array, not the original User entity, and that array is filtered by $allowedFields so the credentials are stripped. What if we override insert() and update() to store credentials in a new private property before calling parent::method() and then still use the after____ methods to handle the identities, just using the stored versions instead?

Added UserModel::saveEmailIdentity() and the Model Events call it when afterInsert and afterUpdate.

MGatner

LGTM!

manukieli · 2022-07-08T08:48:32Z

I try de update the password to existent user and it not working, when the user try to login with new password he get this error message: "Please check your password."
I can update the username, the email, but the password not working to update.

datamweb · 2022-07-08T08:52:38Z

hi @manukieli,
Please see #280.

kenjis · 2022-07-12T07:12:15Z

I rewrote UserModel::save() (and update(), insert()) with the Model Events.
Review please.

MGatner

Thanks for taking a pass at this @kenjis! It didn't end up being quite as clean as I hoped but I still think it is a big improvement. I wish we had the allowEmpty() method, but what can you do 🤷‍♂️

One of the strengths to this approach is that we can support "normal" model operations instead of relying on special use cases with save(). To that end I think that we should still support array parameters, and just have the callback events skip identity updates whenever there is no $tempUser. I'm happy to hear an alternate opinion but I think that improves the transparency of these methods, which I believe was @lonnieezell's original intent.

MGatner · 2022-07-12T11:14:22Z

src/Models/UserModel.php

-     * 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


I think we should still support arrays, and the callback events would just ignore them.

Frankly, I am not sure why it is necessary to accept arrays, but I implemented it that way to try it out.

MGatner · 2022-07-14T10:14:39Z

src/Models/UserModel.php

+            $user->saveEmailIdentity();
+
+            return $data;
+        }
+
+        // Update
+        $this->tempUser->saveEmailIdentity();
+


I know $tempUser is reset on each call but I would feel better if it were also reset after the identity update, unless there is some logical reason to keep it around.

Suggested change

$user->saveEmailIdentity();

return $data;

}

// Update

$this->tempUser->saveEmailIdentity();

$this->tempUser = $user;

}

// Update and reset

$this->tempUser->saveEmailIdentity();

$this->tempUser = null;

Added $this->tempUser = null.

Since we create a User object manually, we need to call it for consistency.

When updating User's email/password, parent::save($data) may return DataException (no data to update).

…vents Remove saveWithEmailIdentity().

…ents do not save Email Identity

MGatner

This one has been a long road! Thanks for all your work on it @kenjis. I believe it's ready!

kenjis · 2022-07-15T22:33:10Z

@MGatner Thank you for the review!

kenjis added the bug Something isn't working label Jul 3, 2022

kenjis marked this pull request as draft July 3, 2022 00:53

kenjis force-pushed the fix-usermodel-save branch from 8cde372 to 19b44c2 Compare July 3, 2022 01:03

MGatner approved these changes Jul 3, 2022

View reviewed changes

kenjis requested a review from MGatner July 3, 2022 21:09

kenjis marked this pull request as ready for review July 4, 2022 06:28

kenjis force-pushed the fix-usermodel-save branch from c811ad5 to 057b221 Compare July 5, 2022 02:26

kenjis force-pushed the fix-usermodel-save branch from 057b221 to 39aaf14 Compare July 5, 2022 02:31

lonnieezell reviewed Jul 5, 2022

View reviewed changes

src/Models/UserModel.php Show resolved Hide resolved

MGatner requested changes Jul 5, 2022

View reviewed changes

src/Entities/User.php Show resolved Hide resolved

src/Controllers/RegisterController.php Outdated Show resolved Hide resolved

kenjis force-pushed the fix-usermodel-save branch from 4581609 to 76b15a5 Compare July 6, 2022 02:10

kenjis requested a review from MGatner July 6, 2022 02:12

lonnieezell reviewed Jul 6, 2022

View reviewed changes

MGatner approved these changes Jul 6, 2022

View reviewed changes

kenjis force-pushed the fix-usermodel-save branch from 4d4ce9d to ed7ff5e Compare July 12, 2022 07:02

kenjis requested review from MGatner and lonnieezell July 12, 2022 07:09

MGatner requested changes Jul 12, 2022

View reviewed changes

kenjis requested a review from MGatner July 14, 2022 00:56

kenjis force-pushed the fix-usermodel-save branch from bc7b647 to d9410fe Compare July 14, 2022 00:59

MGatner requested changes Jul 14, 2022

View reviewed changes

kenjis added 17 commits July 15, 2022 17:26

docs: fix doc comment

2c9367b

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

0f029f6

fix: registerAction() so that it does not insert the identity twice

510f4fb

docs: update the docs

d72ac0a

fix: add missing syncOriginal()

2c01462

Since we create a User object manually, we need to call it for consistency.

fix: can't update User's email/password

7b0145d

When updating User's email/password, parent::save($data) may return DataException (no data to update).

fix: add missing throw $e

c657cae

refactor: change UserModel::save() to saveWithEmailIdentity()

45cd4c4

refactor: remove unnecessary if condition

a3f7e10

fix: add missing checkQueryReturn()

e9604a9

refactor: add ValidationException and use it

95953f4

fix: catch ValidationException

f17355e

feat: UserModel::save() can save Email Identity again

9487136

feat: insert(), update(), save() can save Email Identity with Model E…

51bb61b

…vents Remove saveWithEmailIdentity().

docs: update about UserModel::insert() and update()

af13b4c

feat: when you pass an array user data to insert()/update(), Model ev…

8868006

…ents do not save Email Identity

fix: reset $this->tempUser after save Email Identity

1ecdbdd

kenjis force-pushed the fix-usermodel-save branch from d9410fe to 1ecdbdd Compare July 15, 2022 08:32

kenjis requested a review from MGatner July 15, 2022 08:34

MGatner approved these changes Jul 15, 2022

View reviewed changes

kenjis merged commit 93f607f into codeigniter4:develop Jul 15, 2022

kenjis deleted the fix-usermodel-save branch July 15, 2022 22:32

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

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

Uh oh!

Conversation

kenjis commented Jul 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MGatner left a comment

Choose a reason for hiding this comment

Uh oh!

MGatner commented Jul 3, 2022

Uh oh!

datamweb commented Jul 3, 2022

Uh oh!

kenjis commented Jul 3, 2022

Uh oh!

kenjis commented Jul 3, 2022

Uh oh!

kenjis commented Jul 4, 2022

Uh oh!

kenjis commented Jul 4, 2022

Uh oh!

MGatner commented Jul 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenjis commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MGatner left a comment

Choose a reason for hiding this comment

Uh oh!

manukieli commented Jul 8, 2022

Uh oh!

datamweb commented Jul 8, 2022

Uh oh!

kenjis commented Jul 12, 2022

Uh oh!

MGatner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MGatner left a comment

Choose a reason for hiding this comment

Uh oh!

kenjis commented Jul 15, 2022

Uh oh!

Uh oh!

kenjis commented Jul 3, 2022 •

edited

Loading

MGatner commented Jul 4, 2022 •

edited

Loading

kenjis commented Jul 5, 2022 •

edited

Loading