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

feat: Invalidate a JWT token #1170

Merged
merged 1 commit into from
Apr 15, 2024
Merged

feat: Invalidate a JWT token #1170

merged 1 commit into from
Apr 15, 2024

Conversation

ldaspt
Copy link
Contributor

@ldaspt ldaspt commented Nov 23, 2023

This PR adds support for invalidating a JWT token #1137.

The code comes mainly from the discussion #1005 (reply in thread) Thanks to @mbabker

I think that the PR meets the needs mentioned in #1137 (comment)

  • This feature must be opt-in
  • Tokens should be given a jti claim whose value should be the only thing persisted: if the feature is enabled and a token's jti exists in the blocklist then that token must be rejected.
  • Feature detection should not be only based on the presence of the jti, as it mght break existing code that relies on this claim today.
  • The blacklist term should be avoided, alternative such as blocklist should be preferred :)
  • We will probably need a simple abstraction for the blocklist storage. A very limited set of built-in implementations should be provided, not necessarily as part of the first iteration (i.e. it can wait til another PR).

@chalasr
Copy link
Collaborator

chalasr commented Nov 26, 2023

Nice! Could you please rebase?

@ldaspt
Copy link
Contributor Author

ldaspt commented Nov 27, 2023

@chalasr Rebase done


class AddClaimsToJWTListener
{
public function __invoke(JWTCreatedEvent $event): void
Copy link
Contributor

Choose a reason for hiding this comment

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

For a bundle, this is probably better as a named method instead of being invokable.

Also, maybe Lexik\Bundle\JWTAuthenticationBundle\Services\JWTManager should be updated to automatically add the jti claim to the payload when it creates the token (or at least make it configurable through the bundle config) instead of relying on an event listener. For the folks who don't want the claim or have their own generation logic, they would only need to disable the feature through the config instead of either having to unload the listener through a compiler pass or mess with listener priorities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I liked the implementation via a listener because it avoids any modification in the "JWTManager" code and therefore limits regressions

How do you imagine the modification of "JwtManager" ?

I imagine something like that on my side

diff --git c/Services/JWTManager.php i/Services/JWTManager.php
index b2008bb..754fc9a 100644
--- c/Services/JWTManager.php
+++ i/Services/JWTManager.php
@@ -44,16 +44,19 @@ class JWTManager implements JWTManagerInterface, JWTTokenManagerInterface
      * @var string
      */
     protected $userIdClaim;
+    private $payloadEnrichments;
 
     /**
-     * @param string|null $userIdClaim
+     * @param JWTEncoderInterface $encoder
+     * @param PayloadEnrichmentInterface[] $payloadEnrichments
      */
-    public function __construct(JWTEncoderInterface $encoder, EventDispatcherInterface $dispatcher, $userIdClaim = null)
+    public function __construct(JWTEncoderInterface $encoder, EventDispatcherInterface $dispatcher, $userIdClaim = null, array $payloadEnrichments = [])
     {
         $this->jwtEncoder = $encoder;
         $this->dispatcher = $dispatcher;
         $this->userIdentityField = 'username';
         $this->userIdClaim = $userIdClaim;
+        $this->payloadEnrichments = $payloadEnrichments;
     }
 
     /**
@@ -64,6 +67,8 @@ class JWTManager implements JWTManagerInterface, JWTTokenManagerInterface
         $payload = ['roles' => $user->getRoles()];
         $this->addUserIdentityToPayload($user, $payload);
 
+        $this->enrichPayload($user, $payload);
+
         return $this->generateJwtStringAndDispatchEvents($user, $payload);
     }
 
@@ -75,9 +80,18 @@ class JWTManager implements JWTManagerInterface, JWTTokenManagerInterface
         $payload = array_merge(['roles' => $user->getRoles()], $payload);
         $this->addUserIdentityToPayload($user, $payload);
 
+        $this->enrichPayload($user, $payload);
+
         return $this->generateJwtStringAndDispatchEvents($user, $payload);
     }
 
+    private function enrichPayload(UserInterface $user, array &$payload): void
+    {
+        foreach ($this->payloadEnrichments as $payloadEnrichment) {
+            $payloadEnrichment->enrich($user, $payload);
+        }
+    }
+
     /**
      * @return string The JWT token
      */
diff --git c/Services/PayloadEnrichment/RandomJtiEnrichment.php i/Services/PayloadEnrichment/RandomJtiEnrichment.php
new file mode 100644
index 0000000..82a8926
--- /dev/null
+++ i/Services/PayloadEnrichment/RandomJtiEnrichment.php
@@ -0,0 +1,14 @@
+<?php
+
+namespace Lexik\Bundle\JWTAuthenticationBundle\Services\PayloadEnrichment;
+
+use Lexik\Bundle\JWTAuthenticationBundle\Services\PayloadEnrichmentInterface;
+use Symfony\Component\Security\Core\User\UserInterface;
+
+class RandomJtiEnrichment implements PayloadEnrichmentInterface
+{
+    public function enrich(UserInterface $user, array &$payload): void
+    {
+        $payload['jti'] = bin2hex(random_bytes(16));
+    }
+}
diff --git c/Services/PayloadEnrichmentInterface.php i/Services/PayloadEnrichmentInterface.php
new file mode 100644
index 0000000..5278623
--- /dev/null
+++ i/Services/PayloadEnrichmentInterface.php
@@ -0,0 +1,10 @@
+<?php
+
+namespace Lexik\Bundle\JWTAuthenticationBundle\Services;
+
+use Symfony\Component\Security\Core\User\UserInterface;
+
+interface PayloadEnrichmentInterface
+{
+    public function enrich(UserInterface $user, array &$payload): void;
+}

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that enrichments idea.

For me, the difference between having the JWTManager add the jti claim versus an event listener is that I feel like the manager should build the full payload that it is configured to create and then event listeners are the third-party hook to manipulate that payload. Neither approach is wrong, it just kind of boils down to what folks decide is the approach the core library's going to take.

Comment on lines 21 to 30
try {
if ($this->tokenManager->has($event->getPayload())) {
throw new InvalidTokenException('JWT blocked');
}
} catch (MissingClaimException) {
// Do nothing if the required claims do not exist on the payload (older JWTs won't have the "jti" claim the manager requires)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Handling this check inside the authenticators instead of using an event listener might make a little more sense in the context of a bundle (it works well in an app where I wasn't trying to overload the bundle services/behaviors).

@Vincz
Copy link

Vincz commented Feb 22, 2024

Hi guys! Any ETA regarding this feature? Thanks!

@chalasr
Copy link
Collaborator

chalasr commented Feb 22, 2024

Hey @Vincz :) I'll do my best to move forward with this in the 2 coming weeks, expect it to be released in March

@ldaspt ldaspt force-pushed the blocklist-token branch 2 times, most recently from 89dcc6b to 1dabeaf Compare April 15, 2024 11:53
@ldaspt
Copy link
Contributor Author

ldaspt commented Apr 15, 2024

@chalasr Rebase done, whenever you have some free time, could you please take a look at the pull request? I would really appreciate your feedback and suggestions. Let me know if there's anything specific you'd like me to address.

Thanks!

@chalasr
Copy link
Collaborator

chalasr commented Apr 15, 2024

Thank you @ldaspt.

@chalasr chalasr merged commit 3785c0f into lexik:2.x Apr 15, 2024
9 checks passed
@chalasr
Copy link
Collaborator

chalasr commented Apr 15, 2024

Follow-up PR to give a try to @mbabker's suggestion welcome!

chalasr added a commit that referenced this pull request Apr 27, 2024
…TManager class instead of doing it via a listener (ldaspt)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

Invalidate a JWT token - Adding the jti claim by the JWTManager class instead of doing it via a listener

Hello `@chalasr` and `@mbabker`,

This PR aims to address the enhancements suggested by `@mbabker` in the discussion of the PR #1170.

`@see` #1170 (comment)

Changes included:
* Remove AddClaimsToJWTListener
* Addition of the concept of Payload Enrichment which aims to enrich the payload just before generating the token in the JwtManager class
* Added a random jti when the JWT token invalidation functionality is enabled
* Added a null payload enrichment and a chain enrichment (which may be superfluous)

The payload enrichment cannot be overridden via the bundle configuration, if the developer wants to overload it, he will have to decorate the service `lexik_jwt_authentication.payload_enrichment`. If necessary, I can do as for the `lexik_jwt_authentication.encoder` service so that this is configurable via the bundle configuration

I hope the PR answers the request correctly

Please review the changes at your convenience, and I welcome any feedback or further suggestions

Commits
-------

069b7bb Invalidate a JWT token - Adding the jti claim by the JWTManager class instead of doing it via a listener
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.

4 participants