Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

[Doctrine] fixed security user reloading when the user has been chang…

…ed via a form with validation errors (closes #2033)
  • Loading branch information...
commit 9d2ab9ca9c1762c529e053cefd39a5e626102133 1 parent 414d4be
Fabien Potencier fabpot authored
10 src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php
View
@@ -30,13 +30,15 @@ class EntityUserProvider implements UserProviderInterface
private $class;
private $repository;
private $property;
+ private $metadata;
public function __construct(EntityManager $em, $class, $property = null)
{
$this->class = $class;
+ $this->metadata = $em->getClassMetadata($class);
if (false !== strpos($this->class, ':')) {
- $this->class = $em->getClassMetadata($class)->name;
+ $this->class = $this->metadata->name;
}
$this->repository = $em->getRepository($class);
@@ -74,7 +76,11 @@ public function refreshUser(UserInterface $user)
throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', get_class($user)));
}
- return $this->loadUserByUsername($user->getUsername());
+ // The user must be reloaded via the primary key as all other data
+ // might have changed without proper persistence in the database.
+ // That's the case when the user has been changed by a form with
+ // validation errors.
+ return $this->repository->find($this->metadata->getIdentifierValues($user));
Benjamin Eberlei
beberlei added a note

Just to understand, this will still be a security problem if someone adds the "id" as hidden field through a form instead of going through a get parameter in the Url or?

Fabien Potencier Owner
fabpot added a note

Correct. If you allow the user to change its primary key, then the security issue stil exist. But frankly, I don't think this happens a lot, does it?

Benjamin Eberlei
beberlei added a note

How about additionally saving the username into the token and making an assertion on username == user->getUsername()?

Benjamin Eberlei
beberlei added a note

other than that you are probably right, that this doesnt happen often. The PR looks good to me.

Pierre Durand
pierrre added a note

Since this patch, my authentication is broken.
I use a form authentication, with an email and a password.

With the previous version of this file "$user->getUsername()" returns my user's email.
Currently "$this->metadata->getIdentifierValues($user)" returns an empty array, because the user's id is not saved in the "remember me" cookie. (The user's email is saved).

Is something wrong?

Christophe Coevoet Collaborator
stof added a note

keep the id when serializing your user.

Pierre Durand
pierrre added a note

@stof it works! thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
/**
28 tests/Symfony/Tests/Bridge/Doctrine/Fixtures/CompositeIdentEntity.php
View
@@ -5,9 +5,10 @@
use Doctrine\ORM\Mapping\Id;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
+use Symfony\Component\Security\Core\User\UserInterface;
/** @Entity */
-class CompositeIdentEntity
+class CompositeIdentEntity implements UserInterface
{
/** @Id @Column(type="integer") */
protected $id1;
@@ -23,4 +24,29 @@ public function __construct($id1, $id2, $name) {
$this->id2 = $id2;
$this->name = $name;
}
+
+ public function getRoles()
+ {
+ }
+
+ public function getPassword()
+ {
+ }
+
+ public function getSalt()
+ {
+ }
+
+ public function getUsername()
+ {
+ return $this->name;
+ }
+
+ public function eraseCredentials()
+ {
+ }
+
+ public function equals(UserInterface $user)
+ {
+ }
}
51 tests/Symfony/Tests/Bridge/Doctrine/Security/User/EntityUserProviderTest.php
View
@@ -0,0 +1,51 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Tests\Bridge\Doctrine\Security\User;
+
+require_once __DIR__.'/../../DoctrineOrmTestCase.php';
+require_once __DIR__.'/../../Fixtures/CompositeIdentEntity.php';
+
+use Symfony\Tests\Bridge\Doctrine\DoctrineOrmTestCase;
+use Symfony\Tests\Bridge\Doctrine\Fixtures\CompositeIdentEntity;
+use Symfony\Bridge\Doctrine\Security\User\EntityUserProvider;
+use Doctrine\ORM\Tools\SchemaTool;
+
+class EntityUserProviderTest extends DoctrineOrmTestCase
+{
+ public function testRefreshUserGetsUserByPrimaryKey()
+ {
+ $em = $this->createTestEntityManager();
+ $this->createSchema($em);
+
+ $user1 = new CompositeIdentEntity(1, 1, 'user1');
+ $user2 = new CompositeIdentEntity(1, 2, 'user2');
+
+ $em->persist($user1);
+ $em->persist($user2);
+ $em->flush();
+
+ $provider = new EntityUserProvider($em, 'Symfony\Tests\Bridge\Doctrine\Fixtures\CompositeIdentEntity', 'name');
+
+ // try to change the user identity
+ $user1->name = 'user2';
+
+ $this->assertSame($user1, $provider->refreshUser($user1));
+ }
+
+ private function createSchema($em)
+ {
+ $schemaTool = new SchemaTool($em);
+ $schemaTool->createSchema(array(
+ $em->getClassMetadata('Symfony\Tests\Bridge\Doctrine\Fixtures\CompositeIdentEntity'),
+ ));
+ }
+}
Benjamin Eberlei

Just to understand, this will still be a security problem if someone adds the "id" as hidden field through a form instead of going through a get parameter in the Url or?

Fabien Potencier

Correct. If you allow the user to change its primary key, then the security issue stil exist. But frankly, I don't think this happens a lot, does it?

Benjamin Eberlei

How about additionally saving the username into the token and making an assertion on username == user->getUsername()?

Benjamin Eberlei

other than that you are probably right, that this doesnt happen often. The PR looks good to me.

Pierre Durand

Since this patch, my authentication is broken.
I use a form authentication, with an email and a password.

With the previous version of this file "$user->getUsername()" returns my user's email.
Currently "$this->metadata->getIdentifierValues($user)" returns an empty array, because the user's id is not saved in the "remember me" cookie. (The user's email is saved).

Is something wrong?

Christophe Coevoet

keep the id when serializing your user.

Please sign in to comment.
Something went wrong with that request. Please try again.