search

nineinchnick / yii-usr

Yii framework module for user authentication, password reset, registration and profile updating.
http://demo.niix.pl
MIT License
36 stars 24 forks source link

Login History #31

Closed andrmo closed 10 years ago

andrmo commented 10 years ago

Reviewing this, I notice that the authenticate method in your ExampleUserIdentity class updates the last_visit_on attribute of the ExampleUser model. I know this is an example, but I don't believe that should be the way it works, because:

1) You're effectively creating a side-effect in the authenticate method. I believe that as implemented (and I don't pretend to understand it all!), the authenticate method is not just called at login - for example it's also called when the password is changed.

2) The authenticate method is not called during initial registration.

So the net result would be that last_visit_on would not be updated correctly.

I believe the correct approach if this functionality is needed (and I do need it!) would be to override the afterLogin method of the CWebUser class, or maybe create an additional interface (eg ILoginHistoryIdentity) that ExampleUserIdentity should implement and that would get called from your own afterLogin methods.

nineinchnick commented 10 years ago

Thanks for the review, this is very important. I'll address those issues ASAP. There is a related issue in nineinchnick/yii2-usr#16 about using events.

andrmo commented 10 years ago

OK. I agree that Events would also work for this as in #16

nineinchnick commented 10 years ago

I have to close this one. It is true that last login time should be updated in CWebUser.afterLogin(), not in CUserIdentity.authenticate(). The module provides only an example of user identity and no web user class. If I remove that code from user identity example most people won't know they have to extend CWebUser.

I don't want to introduce an afterLogin method or event in the identity class, because it would just duplicate the on in web user class.

I think the whole confusing situation is caused by the unnecesary splitting of CWebUser and CUserIdentity classes, which is resolved in Yii 2.0.

As for the events, currently they are not needed, because everyone can call more custom login from an appropriate method in user identity class (except from logging in, as described above).

If anybody got some good ideas to improve this please comment and I will reopen the issue.