amnah / yii2-user

Yii2 user authentication module
MIT License
253 stars 104 forks source link

actionLoginCallback fails to load User from $post when useUsername = false #130

Closed cicsolutions closed 8 years ago

cicsolutions commented 8 years ago

Hello,

I think I may have found a bug, although it could be intended functionality, I'm not sure :)

I have a fresh install of the Amnah User module for my testing. I have 'useUsername' => false, as my only custom config setting for the module.

I would like to use the login/register via email functionality so I visit the user/default/login-email view. I enter in my email address and submit the form. Then I check my email and click on the link in the email, which takes me to the user/default/login-callback page.

The login-callback view shows the email field in a disabled state and a field for me to enter my full name. So we are using the User model and the Profile model, however with the 'useUsername' => false setting, there are no other User model fields in the form except for the disabled email field. Therefore, after submitting the form, the controller will not process the form because it cannot load User from the post.

public function actionLoginCallback($token)
{
    // ... code removed here just to focus on where the issue comes in

    // load post data
    $post = Yii::$app->request->post();

    /* Note: $user->load($post) cannot load b/c there are no User fields in the post */
    if ($userToken && $user->load($post)) {

        // ... code removed here just to focus on where the issue comes in

    }

    // ... code removed here just to focus on where the issue comes in
}

Please let me know your thoughts. I may be using the login/register email functionality in a way that is not really intended.

As always, thank you!

P.S. My thoughts on a fix... we probably only need to load the User from the post if the username is being used by the module, right? Since the email address is coming from the UserToken, and there are no other User fields in the form, I think something like the following may address this scenario/situation.

    // load post data
    $post = Yii::$app->request->post();
    if ($userToken) {

        // ensure that email is taken from the $userToken (and not from user input)
        $user->email = $userToken->data;

        // set the username if useUsername = true
        $user->username = $this->module->useUsername ? $post['User']['username'] : null;

        // validate and register
        $profile->load($post);
        if ($user->validate() && $profile->validate()) {
            $role = $this->module->model("Role");
            $user->setRegisterAttributes($role::ROLE_USER, $user::STATUS_ACTIVE)->save();
            $profile->setUser($user->id)->save();

            // log user in and delete token
            $returnUrl = $this->performLogin($user);
            $userToken->delete();
            return $this->redirect($returnUrl);
        }
    }
amnah commented 8 years ago

Yes, this was in fact a bug - thanks for catching it!

Also, what version are you using for this module in composer.json? If you're using "amnah/yii2-user": "^4.0", you might not get this latest change. (and master has a bc-breaking change, which I've been planning to upgrade soon)

cicsolutions commented 8 years ago

Thanks Amnah! I appreciate the heads up on the status of the branches. I'm already overriding the DefaultController in my app, so I'll just replicate your fix in my controller and be happily on my way :)

cicsolutions commented 8 years ago

I hope I'm not annoying you with all my input :)

With the register via email functionality, I decided to set the user model scenario to 'register' to enable the scenario validation rules. I did this in the actionLoginCallback() method, and it was mainly driven by wanting to have the newPassword field included when registering via email.

Not sure if you want this concept/logic in the core or not, but thought I'd put it on your radar just in case.

Cheers!

amnah commented 8 years ago

Ohhhhh... The point of the email functionality was to remove passwords completely (and use email as the only requirement), but I can see how you'd use it as a verification first before registering the account. Never thought of that, interesting!

cicsolutions commented 8 years ago

:) Yep!

I'm working on a module for booking appointments, and I'd like my users to be able to book an appointment regardless of whether or not they have a user account. But if they do want to create a user account, I'm piggy-backing off the register via email functionality to create the userToken and send the user an email. This way, they can complete their appointment booking, and finish setting up their user account later.

Fun, fun, fun!