minor #59558 [Security] Unset token roles when serializing it and user implements EquatableInterface (nicolas-grekas)

nicolas-grekas · nicolas-grekas · commit eff9b5239ccf · 2025-01-29T11:25:30.000+01:00
This PR was merged into the 7.3 branch. Discussion ---------- [Security] Unset token roles when serializing it and user implements EquatableInterface | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT When the user object implement EquatableInterface, we never read the roles stored in the token object that wraps the user in the session storage. This PR ensures we don't store these roles either - they're just wasting space. Commits ------- b7c55c8 [Security] Unset token roles when serializing it and user implements EquatableInterface
diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php
@@ -11,6 +11,7 @@
 
 namespace Symfony\Component\Security\Core\Authentication\Token;
 
+use Symfony\Component\Security\Core\User\EquatableInterface;
 use Symfony\Component\Security\Core\User\InMemoryUser;
 use Symfony\Component\Security\Core\User\UserInterface;
 
@@ -23,24 +24,24 @@
 abstract class AbstractToken implements TokenInterface, \Serializable
 {
     private ?UserInterface $user = null;
-    private array $roleNames = [];
+    private array $roleNames;
     private array $attributes = [];
 
     /**
      * @param string[] $roles An array of roles
-     *
-     * @throws \InvalidArgumentException
      */
     public function __construct(array $roles = [])
     {
+        $this->roleNames = [];
+
         foreach ($roles as $role) {
-            $this->roleNames[] = $role;
+            $this->roleNames[] = (string) $role;
         }
     }
 
     public function getRoleNames(): array
     {
-        return $this->roleNames;
+        return $this->roleNames ??= self::__construct($this->user->getRoles()) ?? $this->roleNames;
     }
 
     public function getUserIdentifier(): string
@@ -82,7 +83,13 @@ public function eraseCredentials(): void
      */
     public function __serialize(): array
     {
-        return [$this->user, true, null, $this->attributes, $this->roleNames];
+        $data = [$this->user, true, null, $this->attributes];
+
+        if (!$this->user instanceof EquatableInterface) {
+            $data[] = $this->roleNames;
+        }
+
+        return $data;
     }
 
     /**
@@ -103,7 +110,12 @@ public function __serialize(): array
      */
     public function __unserialize(array $data): void
     {
-        [$user, , , $this->attributes, $this->roleNames] = $data;
+        [$user, , , $this->attributes] = $data;
+
+        if (\array_key_exists(4, $data)) {
+            $this->roleNames = $data[4];
+        }
+
         $this->user = \is_string($user) ? new InMemoryUser($user, '', $this->roleNames, false) : $user;
     }
 
diff --git a/src/Symfony/Component/Security/Core/Tests/Exception/CustomUserMessageAuthenticationExceptionTest.php b/src/Symfony/Component/Security/Core/Tests/Exception/CustomUserMessageAuthenticationExceptionTest.php
@@ -53,6 +53,7 @@ public function testSharedSerializedData()
         $exception->setSafeMessage('message', ['token' => $token]);
 
         $processed = unserialize(serialize($exception));
+        $this->assertSame($token->getRoleNames(), $processed->getToken()->getRoleNames());
         $this->assertEquals($token, $processed->getToken());
         $this->assertEquals($token, $processed->getMessageData()['token']);
         $this->assertSame($processed->getToken(), $processed->getMessageData()['token']);
@@ -67,6 +68,7 @@ public function testSharedSerializedDataFromChild()
         $exception->setToken($token);
 
         $processed = unserialize(serialize($exception));
+        $this->assertSame($token->getRoleNames(), $processed->getToken()->getRoleNames());
         $this->assertEquals($token, $processed->childMember);
         $this->assertEquals($token, $processed->getToken());
         $this->assertSame($processed->getToken(), $processed->childMember);
diff --git a/src/Symfony/Component/Security/Http/Firewall/ContextListener.php b/src/Symfony/Component/Security/Http/Firewall/ContextListener.php
@@ -308,11 +308,12 @@ private static function hasUserChanged(UserInterface $originalUser, TokenInterfa
             }
         }
 
-        $userRoles = array_map('strval', $refreshedUser->getRoles());
+        $refreshedRoles = array_map('strval', $refreshedUser->getRoles());
+        $originalRoles = $refreshedToken->getRoleNames(); // This comes from cloning the original token, so it still contains the roles of the original user
 
         if (
-            \count($userRoles) !== \count($refreshedToken->getRoleNames())
-            || \count($userRoles) !== \count(array_intersect($userRoles, $refreshedToken->getRoleNames()))
+            \count($refreshedRoles) !== \count($originalRoles)
+            || \count($refreshedRoles) !== \count(array_intersect($refreshedRoles, $originalRoles))
         ) {
             return true;
         }

Original file line number	Diff line number	Diff line change
`@@ -308,11 +308,12 @@ private static function hasUserChanged(UserInterface $originalUser, TokenInterfa`
`308`	`308`	`}`
`309`	`309`	`}`
`310`	`310`
`311`		`- $userRoles = array_map('strval', $refreshedUser->getRoles());`
	`311`	`+ $refreshedRoles = array_map('strval', $refreshedUser->getRoles());`
	`312`	`+ $originalRoles = $refreshedToken->getRoleNames(); // This comes from cloning the original token, so it still contains the roles of the original user`
`312`	`313`
`313`	`314`	`if (`
`314`		`- \count($userRoles) !== \count($refreshedToken->getRoleNames())`
`315`		`- \|\| \count($userRoles) !== \count(array_intersect($userRoles, $refreshedToken->getRoleNames()))`
	`315`	`+ \count($refreshedRoles) !== \count($originalRoles)`
	`316`	`+ \|\| \count($refreshedRoles) !== \count(array_intersect($refreshedRoles, $originalRoles))`
`316`	`317`	`) {`
`317`	`318`	`return true;`
`318`	`319`	`}`