From d9085c63e35bb708f560722fff5f4f5ad322c27b Mon Sep 17 00:00:00 2001 From: Jeremy Date: Tue, 17 Feb 2015 21:03:23 +0100 Subject: [PATCH] Handle password change --- app/config/services.yml | 1 + .../Controller/ConfigController.php | 39 ++++++-- .../DataFixtures/ORM/LoadUserData.php | 4 +- .../Form/Type/ChangePasswordType.php | 40 ++++++++ .../Resources/views/Config/index.html.twig | 58 +++++++++--- .../Encoder/WallabagPasswordEncoder.php | 8 +- .../WallabagUserPasswordValidator.php | 48 ++++++++++ .../Tests/Controller/ConfigControllerTest.php | 94 ++++++++++++++++++- .../Controller/WallabagRestControllerTest.php | 8 +- .../CoreBundle/Tests/WallabagTestCase.php | 2 +- 10 files changed, 268 insertions(+), 34 deletions(-) create mode 100644 src/Wallabag/CoreBundle/Form/Type/ChangePasswordType.php create mode 100644 src/Wallabag/CoreBundle/Security/Validator/WallabagUserPasswordValidator.php diff --git a/app/config/services.yml b/app/config/services.yml index d4485e429..91a03e10e 100644 --- a/app/config/services.yml +++ b/app/config/services.yml @@ -3,6 +3,7 @@ parameters: security.authentication.provider.dao.class: Wallabag\CoreBundle\Security\Authentication\Provider\WallabagAuthenticationProvider security.encoder.digest.class: Wallabag\CoreBundle\Security\Authentication\Encoder\WallabagPasswordEncoder + security.validator.user_password.class: Wallabag\CoreBundle\Security\Validator\WallabagUserPasswordValidator services: # service_name: diff --git a/src/Wallabag/CoreBundle/Controller/ConfigController.php b/src/Wallabag/CoreBundle/Controller/ConfigController.php index f48a9cb19..7540f756c 100644 --- a/src/Wallabag/CoreBundle/Controller/ConfigController.php +++ b/src/Wallabag/CoreBundle/Controller/ConfigController.php @@ -7,6 +7,7 @@ use Symfony\Bundle\FrameworkBundle\Controller\Controller; use Symfony\Component\HttpFoundation\Request; use Wallabag\CoreBundle\Entity\Config; use Wallabag\CoreBundle\Form\Type\ConfigType; +use Wallabag\CoreBundle\Form\Type\ChangePasswordType; class ConfigController extends Controller { @@ -14,19 +15,18 @@ class ConfigController extends Controller * @param Request $request * * @Route("/config", name="config") - * - * @return \Symfony\Component\HttpFoundation\Response */ public function indexAction(Request $request) { + $em = $this->getDoctrine()->getManager(); $config = $this->getConfig(); - $form = $this->createForm(new ConfigType(), $config); + // handle basic config detail + $configForm = $this->createForm(new ConfigType(), $config); + $configForm->handleRequest($request); - $form->handleRequest($request); + if ($configForm->isValid()) { - if ($form->isValid()) { - $em = $this->getDoctrine()->getManager(); $em->persist($config); $em->flush(); @@ -38,11 +38,36 @@ class ConfigController extends Controller return $this->redirect($this->generateUrl('config')); } + // handle changing password + $pwdForm = $this->createForm(new ChangePasswordType()); + $pwdForm->handleRequest($request); + + if ($pwdForm->isValid()) { + $user = $this->getUser(); + $user->setPassword($pwdForm->get('new_password')->getData()); + $em->persist($user); + $em->flush(); + + $this->get('session')->getFlashBag()->add( + 'notice', + 'Password updated' + ); + + return $this->redirect($this->generateUrl('config')); + } + return $this->render('WallabagCoreBundle:Config:index.html.twig', array( - 'form' => $form->createView(), + 'configForm' => $configForm->createView(), + 'pwdForm' => $pwdForm->createView(), )); } + /** + * Retrieve config for the current user. + * If no config were found, create a new one. + * + * @return Wallabag\CoreBundle\Entity\Config + */ private function getConfig() { $config = $this->getDoctrine() diff --git a/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadUserData.php b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadUserData.php index e4751f202..d99412f48 100644 --- a/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadUserData.php +++ b/src/Wallabag/CoreBundle/DataFixtures/ORM/LoadUserData.php @@ -18,7 +18,7 @@ class LoadUserData extends AbstractFixture implements OrderedFixtureInterface $userAdmin->setName('Big boss'); $userAdmin->setEmail('bigboss@wallabag.org'); $userAdmin->setUsername('admin'); - $userAdmin->setPassword('test'); + $userAdmin->setPassword('mypassword'); $manager->persist($userAdmin); @@ -28,7 +28,7 @@ class LoadUserData extends AbstractFixture implements OrderedFixtureInterface $bobUser->setName('Bobby'); $bobUser->setEmail('bobby@wallabag.org'); $bobUser->setUsername('bob'); - $bobUser->setPassword('test'); + $bobUser->setPassword('mypassword'); $manager->persist($bobUser); diff --git a/src/Wallabag/CoreBundle/Form/Type/ChangePasswordType.php b/src/Wallabag/CoreBundle/Form/Type/ChangePasswordType.php new file mode 100644 index 000000000..8bf4ca1ef --- /dev/null +++ b/src/Wallabag/CoreBundle/Form/Type/ChangePasswordType.php @@ -0,0 +1,40 @@ +add('old_password', 'password', array( + 'constraints' => new UserPassword(array('message' => 'Wrong value for your current password')), + )) + ->add('new_password', 'repeated', array( + 'type' => 'password', + 'invalid_message' => 'The password fields must match.', + 'required' => true, + 'first_options' => array('label' => 'New password'), + 'second_options' => array('label' => 'Repeat new password'), + 'constraints' => array( + new Constraints\Length(array( + 'min' => 6, + 'minMessage' => 'Password should by at least 6 chars long' + )), + new Constraints\NotBlank() + ) + )) + ->add('save', 'submit') + ; + } + + public function getName() + { + return 'change_passwd'; + } +} diff --git a/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig b/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig index 9c04ff206..941451778 100644 --- a/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/Config/index.html.twig @@ -7,35 +7,67 @@ {% endblock %} {% block content %} -

Basic config

+

{% trans %}Basic config{% endtrans %}

-
- {{ form_errors(form) }} + + {{ form_errors(configForm) }}
- {{ form_label(form.theme) }} - {{ form_errors(form.theme) }} - {{ form_widget(form.theme) }} + {{ form_label(configForm.theme) }} + {{ form_errors(configForm.theme) }} + {{ form_widget(configForm.theme) }}
- {{ form_label(form.items_per_page) }} - {{ form_errors(form.items_per_page) }} - {{ form_widget(form.items_per_page) }} + {{ form_label(configForm.items_per_page) }} + {{ form_errors(configForm.items_per_page) }} + {{ form_widget(configForm.items_per_page) }}
- {{ form_label(form.language) }} - {{ form_errors(form.language) }} - {{ form_widget(form.language) }} + {{ form_label(configForm.language) }} + {{ form_errors(configForm.language) }} + {{ form_widget(configForm.language) }}
- {{ form_rest(form) }} + {{ form_rest(configForm) }} +
+ +

{% trans %}Change your password{% endtrans %}

+ +
+ {{ form_errors(pwdForm) }} + +
+
+ {{ form_label(pwdForm.old_password) }} + {{ form_errors(pwdForm.old_password) }} + {{ form_widget(pwdForm.old_password) }} +
+
+ +
+
+ {{ form_label(pwdForm.new_password.first) }} + {{ form_errors(pwdForm.new_password.first) }} + {{ form_widget(pwdForm.new_password.first) }} +
+
+ +
+
+ {{ form_label(pwdForm.new_password.second) }} + {{ form_errors(pwdForm.new_password.second) }} + {{ form_widget(pwdForm.new_password.second) }} +
+
+ + {{ form_rest(pwdForm) }}
{% endblock %} diff --git a/src/Wallabag/CoreBundle/Security/Authentication/Encoder/WallabagPasswordEncoder.php b/src/Wallabag/CoreBundle/Security/Authentication/Encoder/WallabagPasswordEncoder.php index 56f1affe3..fcfe418bf 100644 --- a/src/Wallabag/CoreBundle/Security/Authentication/Encoder/WallabagPasswordEncoder.php +++ b/src/Wallabag/CoreBundle/Security/Authentication/Encoder/WallabagPasswordEncoder.php @@ -41,10 +41,6 @@ class WallabagPasswordEncoder extends BasePasswordEncoder */ public function encodePassword($raw, $salt) { - if (null === $this->username) { - throw new \LogicException('We can not check the password without a username.'); - } - if ($this->isPasswordTooLong($raw)) { throw new BadCredentialsException('Invalid password.'); } @@ -71,6 +67,10 @@ class WallabagPasswordEncoder extends BasePasswordEncoder */ protected function mergePasswordAndSalt($password, $salt) { + if (null === $this->username) { + throw new \LogicException('We can not check the password without a username.'); + } + if (empty($salt)) { return $password; } diff --git a/src/Wallabag/CoreBundle/Security/Validator/WallabagUserPasswordValidator.php b/src/Wallabag/CoreBundle/Security/Validator/WallabagUserPasswordValidator.php new file mode 100644 index 000000000..5586f9762 --- /dev/null +++ b/src/Wallabag/CoreBundle/Security/Validator/WallabagUserPasswordValidator.php @@ -0,0 +1,48 @@ +securityContext = $securityContext; + $this->encoderFactory = $encoderFactory; + } + + /** + * {@inheritdoc} + */ + public function validate($password, Constraint $constraint) + { + if (!$constraint instanceof UserPassword) { + throw new UnexpectedTypeException($constraint, __NAMESPACE__.'\UserPassword'); + } + + $user = $this->securityContext->getToken()->getUser(); + + if (!$user instanceof UserInterface) { + throw new ConstraintDefinitionException('The User object must implement the UserInterface interface.'); + } + + // give username, it's used to hash the password + $encoder = $this->encoderFactory->getEncoder($user); + $encoder->setUsername($user->getUsername()); + + if (!$encoder->isPasswordValid($user->getPassword(), $password, $user->getSalt())) { + $this->context->addViolation($constraint->message); + } + } +} diff --git a/src/Wallabag/CoreBundle/Tests/Controller/ConfigControllerTest.php b/src/Wallabag/CoreBundle/Tests/Controller/ConfigControllerTest.php index 30809a040..4aceed150 100644 --- a/src/Wallabag/CoreBundle/Tests/Controller/ConfigControllerTest.php +++ b/src/Wallabag/CoreBundle/Tests/Controller/ConfigControllerTest.php @@ -26,7 +26,8 @@ class ConfigControllerTest extends WallabagTestCase $this->assertEquals(200, $client->getResponse()->getStatusCode()); $this->assertCount(1, $crawler->filter('input[type=number]')); - $this->assertCount(1, $crawler->filter('button[type=submit]')); + $this->assertCount(1, $crawler->filter('button[id=config_save]')); + $this->assertCount(1, $crawler->filter('button[id=change_passwd_save]')); } public function testUpdate() @@ -38,7 +39,7 @@ class ConfigControllerTest extends WallabagTestCase $this->assertEquals(200, $client->getResponse()->getStatusCode()); - $form = $crawler->filter('button[type=submit]')->form(); + $form = $crawler->filter('button[id=config_save]')->form(); $data = array( 'config[theme]' => 'baggy', @@ -84,7 +85,7 @@ class ConfigControllerTest extends WallabagTestCase $this->assertEquals(200, $client->getResponse()->getStatusCode()); - $form = $crawler->filter('button[type=submit]')->form(); + $form = $crawler->filter('button[id=config_save]')->form(); $crawler = $client->submit($form, $data); @@ -93,4 +94,91 @@ class ConfigControllerTest extends WallabagTestCase $this->assertGreaterThan(1, $alert = $crawler->filter('body')->extract(array('_text'))); $this->assertContains('This value should not be blank', $alert[0]); } + + public function dataForChangePasswordFailed() + { + return array( + array( + array( + 'change_passwd[old_password]' => 'baggy', + 'change_passwd[new_password][first]' => '', + 'change_passwd[new_password][second]' => '', + ), + 'Wrong value for your current password' + ), + array( + array( + 'change_passwd[old_password]' => 'mypassword', + 'change_passwd[new_password][first]' => '', + 'change_passwd[new_password][second]' => '', + ), + 'This value should not be blank' + ), + array( + array( + 'change_passwd[old_password]' => 'mypassword', + 'change_passwd[new_password][first]' => 'hop', + 'change_passwd[new_password][second]' => '', + ), + 'The password fields must match' + ), + array( + array( + 'change_passwd[old_password]' => 'mypassword', + 'change_passwd[new_password][first]' => 'hop', + 'change_passwd[new_password][second]' => 'hop', + ), + 'Password should by at least 6 chars long' + ), + ); + } + + /** + * @dataProvider dataForChangePasswordFailed + */ + public function testChangePasswordFailed($data, $expectedMessage) + { + $this->logInAs('admin'); + $client = $this->getClient(); + + $crawler = $client->request('GET', '/config'); + + $this->assertEquals(200, $client->getResponse()->getStatusCode()); + + $form = $crawler->filter('button[id=change_passwd_save]')->form(); + + $crawler = $client->submit($form, $data); + + $this->assertEquals(200, $client->getResponse()->getStatusCode()); + + $this->assertGreaterThan(1, $alert = $crawler->filter('body')->extract(array('_text'))); + $this->assertContains($expectedMessage, $alert[0]); + } + + public function testChangePassword() + { + $this->logInAs('admin'); + $client = $this->getClient(); + + $crawler = $client->request('GET', '/config'); + + $this->assertEquals(200, $client->getResponse()->getStatusCode()); + + $form = $crawler->filter('button[id=change_passwd_save]')->form(); + + $data = array( + 'change_passwd[old_password]' => 'mypassword', + 'change_passwd[new_password][first]' => 'mypassword', + 'change_passwd[new_password][second]' => 'mypassword', + ); + + $client->submit($form, $data); + + $this->assertEquals(302, $client->getResponse()->getStatusCode()); + + $crawler = $client->followRedirect(); + + $this->assertGreaterThan(1, $alert = $crawler->filter('div.flash-notice')->extract(array('_text'))); + $this->assertContains('Password updated', $alert[0]); + } } diff --git a/src/Wallabag/CoreBundle/Tests/Controller/WallabagRestControllerTest.php b/src/Wallabag/CoreBundle/Tests/Controller/WallabagRestControllerTest.php index d77e23037..fcfa8ccf9 100644 --- a/src/Wallabag/CoreBundle/Tests/Controller/WallabagRestControllerTest.php +++ b/src/Wallabag/CoreBundle/Tests/Controller/WallabagRestControllerTest.php @@ -47,7 +47,7 @@ class WallabagRestControllerTest extends WallabagTestCase $client->request('GET', '/api/salts/admin.json'); $salt = json_decode($client->getResponse()->getContent()); - $headers = $this->generateHeaders('admin', 'test', $salt[0]); + $headers = $this->generateHeaders('admin', 'mypassword', $salt[0]); $entry = $client->getContainer() ->get('doctrine.orm.entity_manager') @@ -73,7 +73,7 @@ class WallabagRestControllerTest extends WallabagTestCase $client->request('GET', '/api/salts/admin.json'); $salt = json_decode($client->getResponse()->getContent()); - $headers = $this->generateHeaders('admin', 'test', $salt[0]); + $headers = $this->generateHeaders('admin', 'mypassword', $salt[0]); $entry = $client->getContainer() ->get('doctrine.orm.entity_manager') @@ -101,7 +101,7 @@ class WallabagRestControllerTest extends WallabagTestCase $client->request('GET', '/api/salts/admin.json'); $salt = json_decode($client->getResponse()->getContent()); - $headers = $this->generateHeaders('admin', 'test', $salt[0]); + $headers = $this->generateHeaders('admin', 'mypassword', $salt[0]); $client->request('GET', '/api/entries', array(), array(), $headers); @@ -125,7 +125,7 @@ class WallabagRestControllerTest extends WallabagTestCase $client->request('GET', '/api/salts/admin.json'); $salt = json_decode($client->getResponse()->getContent()); - $headers = $this->generateHeaders('admin', 'test', $salt[0]); + $headers = $this->generateHeaders('admin', 'mypassword', $salt[0]); $entry = $client->getContainer() ->get('doctrine.orm.entity_manager') diff --git a/src/Wallabag/CoreBundle/Tests/WallabagTestCase.php b/src/Wallabag/CoreBundle/Tests/WallabagTestCase.php index 397945459..22016d8ed 100644 --- a/src/Wallabag/CoreBundle/Tests/WallabagTestCase.php +++ b/src/Wallabag/CoreBundle/Tests/WallabagTestCase.php @@ -24,7 +24,7 @@ abstract class WallabagTestCase extends WebTestCase $form = $crawler->filter('button[type=submit]')->form(); $data = array( '_username' => $username, - '_password' => 'test', + '_password' => 'mypassword', ); $this->client->submit($form, $data);