Skip to content

Commit

Permalink
Changed the refreshing of the user to load the user by primary key
Browse files Browse the repository at this point in the history
This fixes a security issue if the user is allowed to change his
username and enters an invalid one as this new name would be stored in
the session and used by the next request.
  • Loading branch information
stof committed Jul 10, 2012
1 parent b3dc5fa commit 5a36e29
Showing 1 changed file with 9 additions and 1 deletion.
10 changes: 9 additions & 1 deletion Model/UserManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,16 @@ public function refreshUser(SecurityUserInterface $user)
if (!$user instanceof $class) {
throw new UnsupportedUserException('Account is not supported.');
}
if (!$user instanceof User) {
throw new UnsupportedUserException(sprintf('Expected an instance of FOS\UserBundle\Model\User, but got "%s".', get_class($user)));

This comment has been minimized.

Copy link
@DenisGorbachev

DenisGorbachev Dec 1, 2012

Is it necessary to enforce FOS\UserBundle\Model\User? Maybe FOS\UserBundle\Model\UserInterface would suffice?

I've written my own user class that doesn't extend FOS\UserBundle\Model\User, but is perfectly valid for this provider.

This comment has been minimized.

Copy link
@stof

stof Dec 1, 2012

Author Member

UserInterface does not have an id

This comment has been minimized.

Copy link
@mvrhov

mvrhov Dec 2, 2012

Why don't you extend the FOS\UserBundle\Model\User ?

This comment has been minimized.

Copy link
@DenisGorbachev

DenisGorbachev Dec 3, 2012

@stof I thought it was implied that I can use my own User class without extending the one provided by the bundle. Maybe it would be better to write a DAOUserInterface, which would have an id?

This comment has been minimized.

Copy link
@DenisGorbachev

DenisGorbachev Dec 3, 2012

@mvrhov I need to have another parent for my User class.

This comment has been minimized.

Copy link
@stof

stof Dec 3, 2012

Author Member

@DenisGorbachev you can, but you will need to extend the UserManager for now if you use a different parent class. I plan to change it but I don't have much time to work on the bundle right now (otherwise, #867 would have been updated to finish it)

This comment has been minimized.

Copy link
@DenisGorbachev

DenisGorbachev Dec 3, 2012

Should I post an issue?

}

$user = $this->findUserBy(array('id' => $user->getId()));
if (null === $user) {
throw new UsernameNotFoundException(sprintf('User with ID "%d" could not be reloaded.', $user->getId()));

This comment has been minimized.

Copy link
@schmittjoh

schmittjoh Jul 10, 2012

$user on this line is null and will produce an error.

}

return $this->loadUserByUsername($user->getUsername());
return $user;
}

/**
Expand Down

0 comments on commit 5a36e29

Please sign in to comment.