Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a login event for login counter and flash message for login success. #120

Merged
merged 2 commits into from
Jul 11, 2018
Merged

Add a login event for login counter and flash message for login success. #120

merged 2 commits into from
Jul 11, 2018

Conversation

dereuromark
Copy link
Contributor

The normal login code is not fired/executed at all for hybrid auth login:

/**
 * @return \Cake\Http\Response|null
 */
public function login()
{
    if ($this->request->is('post')) {
        $user = $this->Auth->identify();
        if ($user) {
            unset($user['password']);
            $this->Auth->setUser($user);
            $this->Users->loginUpdate($user);

            $this->Flash->success(__('You are now logged in'));
            return $this->redirect($this->Auth->redirectUrl());
        }
        $this->Flash->error(__('Login or password was not correct'));
    }
}

As such no flash message for successful login or login counter update can happen right now.

This adds the missing event to be able to enable this (to sync with normal post form login).

@@ -50,6 +50,8 @@ public function authenticated()
if ($user) {
$this->Auth->setUser($user);

$this->dispatchEvent('HybridAuth.login', [$user]);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change [$user] to ['user' => $user] for easier accessibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?
This is passed as argument, so there is no key here anyway.

Copy link
Contributor Author

@dereuromark dereuromark Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs I added show it used as

public function updateUser(Event $event, array $user)
{
    $this->loginUpdate($user);
}

protected function loginUpdate(array $user) 
{
    return (bool)$this->updateAll(['last_login' => new FrozenTime()], ['id' => $user['id']]);
}

Copy link
Owner

@ADmad ADmad Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's passed as argument but using key also allows accessing it as $event->getData()['user']. I have done similar when 'HybridAuth.newUser' event is dispatched from the authenticate class so let's stay consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see both used in core, I guess for debugging etc it is easier to have the assoc key? OK

@dereuromark
Copy link
Contributor Author

Done :)

@ADmad ADmad merged commit 9864e06 into ADmad:master Jul 11, 2018
@dereuromark dereuromark deleted the feature/login-event branch July 11, 2018 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants