raise default auth events #45

okaufmann · 2021-11-10T15:16:48Z

Hi,

The Laravel SessionGuard dispatches several events, which can be used to listen when a user logs in etc...

JWTGuard already dispatched some of these events and I just added the other ones, that make sense.

Messhias · 2021-11-10T15:19:47Z

Hi @okaufmann , thanks for your contribution, can you add some tests case to cover your code? Doesn't need to be very complex, but let's keep the code coverage (or at least most of it).

If it's not possible please let me know.

Thank you,

okaufmann · 2021-11-10T15:51:51Z

Hi @Messhias

Thanks for your prompt response!

Of course, I extended the tests for these events.

Let me know if I forgot something.

mfn

Hi 👋🏼 ,

thanks for the PR, overall LGTM but it's clear some of the comments are a copypaste from somewhere else (SessionGuard?); nothing wrong with that, but they're not technical correct.

WDYT about my feedback?

mfn · 2021-11-10T19:37:29Z

src/JWTGuard.php

+
+

Can you please remove those empty lines?

mfn · 2021-11-10T19:40:07Z

src/JWTGuard.php

@@ -167,6 +170,11 @@ public function login(JWTSubject $user)
        $token = $this->jwt->fromUser($user);
        $this->setToken($token)->setUser($user);

+        // If we have an event dispatcher instance set we will fire an event so that


If we have an event dispatcher instance set

I don't read anything in the code where the event dispatcher is option; it's injected via DI and is required, so I think the comment isn't technically correct.

My other feedback here is: the method is called fireLoginEvent

The comment spawns 3 lines essentially telling us this.

My TL;DR feedback here would: method name is good and very clear, we don't need the comment at all

Well, i did just copy this from the SessionGuard without thinking about it. I removed it ^^

mfn · 2021-11-10T19:42:14Z

src/JWTGuard.php

+        // If we have an event dispatcher instance, we can fire off the logout event
+        // so any further processing can be done. This allows the developer to be
+        // listening for anytime a user signs out of this application manually.
+        $this->events->dispatch(new Logout($this->name, $this->user));


It feels inconsistent when all other events are wrapped in dedicated protected fire*Event methods but this one isn't.

My suggestion:

remove the comment (same reason as with the other comment)

add a dedicated fireLogoutEvent

mfn · 2021-11-10T19:45:23Z

src/JWTGuard.php

@@ -501,6 +533,33 @@ protected function fireFailedEvent($user, array $credentials)
        ));
    }

+    /**
+     * Fire the authenticated event if the dispatcher is set.


if the dispatcher is set.

This isn't correct: the dispatcher is always present

mfn · 2021-11-10T19:45:28Z

src/JWTGuard.php

+    }
+
+    /**
+     * Fire the login event if the dispatcher is set.


if the dispatcher is set.

This isn't correct: the dispatcher is always present

okaufmann · 2021-11-11T13:02:36Z

@mfn Hi Markus, you are absolutely right! Thanks for your time and review.

Sorry for the shitty PR, I cleaned up all the things you mentioned and hope it's now clean and correct.

mfn

Sorry for the shitty PR

FTR, I don't think so at all!

Also, thanks for initial version which prompted my code style comment; it prompted me to do #46 which was already accepted 😏

LGTM!

ps: I'm just a contributor like you, not a member, cannot merge stuff

mfn · 2021-11-11T14:59:29Z

src/JWTGuard.php

+    /**
+     * Set the current user.
+     *
+     * @param \Illuminate\Contracts\Auth\Authenticatable $user
+     * @return $this
+     */
+    public function setUser(Authenticatable $user)
+    {
+        $this->user = $user;
+
+        $this->fireAuthenticatedEvent($user);
+
+        return $this;
+    }
+


Just a note for myself: setUser is part of the interface and was covered by a trait so far (that's why it's not in this class), this basically re-implements it and adds the event dispatch.

Technically we could still call the trait version via an alias to not have redundant code, but I guess for the user set this would take things to far 😄

So it isn't better to move straight to trait and do everything in this PR?

sorry, "parser error" 😬 Can you re-phrase maybe?

Laravel also overwrites setUser for the same reason: https://github.com/laravel/framework/blob/8.x/src/Illuminate/Auth/SessionGuard.php#L923

That's why I did it the same way...

The goal was to raise the same events as SessionGuard where it is possible, and for setUser an Authenticated event should be raised.

@mfn on that case it's seems we're facing a limitation of the Laravel framework for setUser since it's overridden.

@okaufmann does that would harm the library? As I far I did see any problem at all.

Hmm you're right, this is a risk when Laravel changes the behaviour of setUser in its Auth Guards. But this is not a breaking change at all, as it does only raise an event now. When no one listens for this event, nothing happen. Maybe you should note it in the Release notes.

So I think we can risk it. What do you think?

well yes, this is what I was insinuating with the alias; to be on the safe side we could do this (untested):

diff --git a/src/JWTGuard.php b/src/JWTGuard.php index ba02510..b89a0ab 100644 --- a/src/JWTGuard.php +++ b/src/JWTGuard.php @@ -31,7 +31,10 @@ use PHPOpenSourceSaver\JWTAuth\Exceptions\UserNotDefinedException; class JWTGuard implements Guard { - use GuardHelpers, Macroable { + use GuardHelpers { + setUser as guardHelperSetUser; + } + use Macroable { __call as macroCall; } @@ -385,11 +388,11 @@ class JWTGuard implements Guard */ public function setUser(Authenticatable $user) { - $this->user = $user; + $result = $this->guardHelperSetUser($user); $this->fireAuthenticatedEvent($user); - return $this; + return $result; } /**

WDYT?

Great idea! Seems to work as well.

mfn

👍🏼

okaufmann · 2021-11-15T16:50:48Z

@Messhias Please let me know if there is anything I can do so this can be merged and release 💪

Messhias · 2021-11-16T10:37:43Z

@Messhias Please let me know if there is anything I can do so this can be merged and release 💪

All the tests failed, can you check that?

okaufmann · 2021-11-16T12:09:24Z

@Messhias Completed the tests and fixed duplicate Attempting event.

Can you trigger the tests again?

Messhias · 2021-11-16T15:10:22Z

@Messhias Completed the tests and fixed duplicate Attempting event.

Can you trigger the tests again?

Great work, thanks :)

okaufmann · 2021-11-16T15:49:43Z

@Messhias would you mind creating a new release containing these changes?

Messhias · 2021-11-16T16:07:35Z

@Messhias would you mind creating a new release containing these changes?

No problem, me and @eschricker we'll do it.

okaufmann force-pushed the feature/raise-default-auth-events branch 2 times, most recently from e476d91 to 85d452b Compare November 10, 2021 15:49

mfn suggested changes Nov 10, 2021

View reviewed changes

mfn mentioned this pull request Nov 10, 2021

Add php-cs-fixer with PSR12 as baseline + github workflow to auto-fix the code style #46

Merged

okaufmann force-pushed the feature/raise-default-auth-events branch 2 times, most recently from 7281f7d to c097cd9 Compare November 11, 2021 12:59

mfn approved these changes Nov 11, 2021

View reviewed changes

okaufmann force-pushed the feature/raise-default-auth-events branch 2 times, most recently from 7ac7755 to dd81b7d Compare November 12, 2021 13:11

mfn approved these changes Nov 12, 2021

View reviewed changes

raise default auth events

dec077e

okaufmann force-pushed the feature/raise-default-auth-events branch from dd81b7d to dec077e Compare November 16, 2021 12:08

Messhias approved these changes Nov 16, 2021

View reviewed changes

Messhias merged commit f68d286 into PHP-Open-Source-Saver:develop Nov 16, 2021

okaufmann deleted the feature/raise-default-auth-events branch November 16, 2021 15:49

Geoffry304 mentioned this pull request May 31, 2022

"Auth guard [tymon.jwt] is not defined" with adldap2-laravel #120

Closed

djurovicigoor mentioned this pull request Oct 6, 2022

Method [viaRemember] does not exist. #193

Closed

maxdiable mentioned this pull request Feb 3, 2023

"exception":"[object] (PHPOpenSourceSaver\\JWTAuth\\Exceptions\\SecretMissingException(code: 0) #206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raise default auth events #45

raise default auth events #45

okaufmann commented Nov 10, 2021 •

edited

Loading

Messhias commented Nov 10, 2021

okaufmann commented Nov 10, 2021 •

edited

Loading

mfn left a comment

mfn Nov 10, 2021

okaufmann Nov 11, 2021

mfn Nov 10, 2021

okaufmann Nov 11, 2021 •

edited

Loading

mfn Nov 10, 2021

okaufmann Nov 11, 2021

mfn Nov 10, 2021

okaufmann Nov 11, 2021

mfn Nov 10, 2021

okaufmann Nov 11, 2021

okaufmann commented Nov 11, 2021

mfn left a comment

mfn Nov 11, 2021

Messhias Nov 11, 2021

mfn Nov 11, 2021

okaufmann Nov 11, 2021 •

edited

Loading

Messhias Nov 11, 2021

okaufmann Nov 11, 2021

mfn Nov 12, 2021

okaufmann Nov 12, 2021

mfn left a comment

okaufmann commented Nov 15, 2021

Messhias commented Nov 16, 2021

okaufmann commented Nov 16, 2021 •

edited

Loading

Messhias commented Nov 16, 2021

okaufmann commented Nov 16, 2021

Messhias commented Nov 16, 2021

raise default auth events #45

raise default auth events #45

Conversation

okaufmann commented Nov 10, 2021 • edited Loading

Messhias commented Nov 10, 2021

okaufmann commented Nov 10, 2021 • edited Loading

mfn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okaufmann Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okaufmann commented Nov 11, 2021

mfn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okaufmann Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfn left a comment

Choose a reason for hiding this comment

okaufmann commented Nov 15, 2021

Messhias commented Nov 16, 2021

okaufmann commented Nov 16, 2021 • edited Loading

Messhias commented Nov 16, 2021

okaufmann commented Nov 16, 2021

Messhias commented Nov 16, 2021

okaufmann commented Nov 10, 2021 •

edited

Loading

okaufmann commented Nov 10, 2021 •

edited

Loading

okaufmann Nov 11, 2021 •

edited

Loading

okaufmann Nov 11, 2021 •

edited

Loading

okaufmann commented Nov 16, 2021 •

edited

Loading