bug #37008 [Security] Fixed AbstractToken::hasUserChanged() (wouterj)

nicolas-grekas · nicolas-grekas · commit bdb01db3dcd5 · 2020-05-30T23:50:18.000+02:00
This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [Security] Fixed AbstractToken::hasUserChanged() | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #36989 | License | MIT | Doc PR | - This PR completely reverts #35944. That PR tried to fix a BC break (ref #35941, #35509) introduced by #31177. However, this broke many authentications (ref #36989), as the User is serialized in the session (as hinted by @stof). Many applications don't include the `roles` property in the serialization (at least, the MakerBundle doesn't include it). In 5.2, we should probably deprecate having different roles in token and user, which fixes the BC breaks all together. Commits ------- f297beb [Security] Fixed AbstractToken::hasUserChanged()
diff --git a/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php b/src/Symfony/Component/Security/Core/Authentication/Token/AbstractToken.php
@@ -317,10 +317,13 @@ private function hasUserChanged(UserInterface $user): bool
             return true;
         }
 
-        $currentUserRoles = array_map('strval', (array) $this->user->getRoles());
         $userRoles = array_map('strval', (array) $user->getRoles());
 
-        if (\count($userRoles) !== \count($currentUserRoles) || \count($userRoles) !== \count(array_intersect($userRoles, $currentUserRoles))) {
+        if ($this instanceof SwitchUserToken) {
+            $userRoles[] = 'ROLE_PREVIOUS_ADMIN';
+        }
+
+        if (\count($userRoles) !== \count($this->getRoleNames()) || \count($userRoles) !== \count(array_intersect($userRoles, $this->getRoleNames()))) {
             return true;
         }
 
diff --git a/src/Symfony/Component/Security/Core/Tests/Authentication/Token/AbstractTokenTest.php b/src/Symfony/Component/Security/Core/Tests/Authentication/Token/AbstractTokenTest.php
@@ -238,13 +238,28 @@ public function getUserChangesAdvancedUser()
      */
     public function testSetUserDoesNotSetAuthenticatedToFalseWhenUserDoesNotChange($user)
     {
-        $token = new ConcreteToken(['ROLE_FOO']);
+        $token = new ConcreteToken();
+        $token->setAuthenticated(true);
+        $this->assertTrue($token->isAuthenticated());
+
+        $token->setUser($user);
+        $this->assertTrue($token->isAuthenticated());
+
+        $token->setUser($user);
+        $this->assertTrue($token->isAuthenticated());
+    }
+
+    public function testIsUserChangedWhenSerializing()
+    {
+        $token = new ConcreteToken(['ROLE_ADMIN']);
         $token->setAuthenticated(true);
         $this->assertTrue($token->isAuthenticated());
 
+        $user = new SerializableUser('wouter', ['ROLE_ADMIN']);
         $token->setUser($user);
         $this->assertTrue($token->isAuthenticated());
 
+        $token = unserialize(serialize($token));
         $token->setUser($user);
         $this->assertTrue($token->isAuthenticated());
     }
@@ -265,6 +280,56 @@ public function __toString(): string
     }
 }
 
+class SerializableUser implements UserInterface, \Serializable
+{
+    private $roles;
+    private $name;
+
+    public function __construct($name, array $roles = [])
+    {
+        $this->name = $name;
+        $this->roles = $roles;
+    }
+
+    public function getUsername()
+    {
+        return $this->name;
+    }
+
+    public function getPassword()
+    {
+        return '***';
+    }
+
+    public function getRoles()
+    {
+        if (empty($this->roles)) {
+            return ['ROLE_USER'];
+        }
+
+        return $this->roles;
+    }
+
+    public function eraseCredentials()
+    {
+    }
+
+    public function getSalt()
+    {
+        return null;
+    }
+
+    public function serialize()
+    {
+        return serialize($this->name);
+    }
+
+    public function unserialize($serialized)
+    {
+        $this->name = unserialize($serialized);
+    }
+}
+
 class ConcreteToken extends AbstractToken
 {
     private $credentials = 'credentials_value';

Original file line number	Diff line number	Diff line change
`@@ -317,10 +317,13 @@ private function hasUserChanged(UserInterface $user): bool`
`317`	`317`	`return true;`
`318`	`318`	`}`
`319`	`319`
`320`		`- $currentUserRoles = array_map('strval', (array) $this->user->getRoles());`
`321`	`320`	`$userRoles = array_map('strval', (array) $user->getRoles());`
`322`	`321`
`323`		`- if (\count($userRoles) !== \count($currentUserRoles) \|\| \count($userRoles) !== \count(array_intersect($userRoles, $currentUserRoles))) {`
	`322`	`+ if ($this instanceof SwitchUserToken) {`
	`323`	`+ $userRoles[] = 'ROLE_PREVIOUS_ADMIN';`
	`324`	`+ }`
	`325`	`+`
	`326`	`+ if (\count($userRoles) !== \count($this->getRoleNames()) \|\| \count($userRoles) !== \count(array_intersect($userRoles, $this->getRoleNames()))) {`
`324`	`327`	`return true;`
`325`	`328`	`}`
`326`	`329`