From bead8b42da4f17238dc0d5e0f90184b224ec5df7 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Wed, 14 Jun 2017 15:02:34 +0200 Subject: [PATCH] Fix reviews Encrypt username too Redirect to list after saving credentials Fix typos Signed-off-by: Thomas Citharel --- .../Version20170501115751.php | 2 +- .../Controller/SiteCredentialController.php | 38 ++++++++++++++----- .../CoreBundle/Entity/SiteCredential.php | 3 +- .../Form/Type/SiteCredentialType.php | 1 + .../GrabySiteConfigBuilder.php | 3 +- .../CoreBundle/Helper/CryptoProxy.php | 4 +- .../Repository/SiteCredentialRepository.php | 3 +- .../Resources/translations/messages.fr.yml | 4 +- .../material/SiteCredential/index.html.twig | 4 +- .../Controller/EntryControllerTest.php | 2 +- .../SiteCredentialControllerTest.php | 1 - 11 files changed, 43 insertions(+), 22 deletions(-) diff --git a/app/DoctrineMigrations/Version20170501115751.php b/app/DoctrineMigrations/Version20170501115751.php index 2597f1ffd..7f068eb84 100644 --- a/app/DoctrineMigrations/Version20170501115751.php +++ b/app/DoctrineMigrations/Version20170501115751.php @@ -38,7 +38,7 @@ class Version20170501115751 extends AbstractMigration implements ContainerAwareI $table->addColumn('id', 'integer', ['autoincrement' => true]); $table->addColumn('user_id', 'integer'); $table->addColumn('host', 'string', ['length' => 255]); - $table->addColumn('username', 'string', ['length' => 255]); + $table->addColumn('username', 'text'); $table->addColumn('password', 'text'); $table->addColumn('createdAt', 'datetime'); $table->addIndex(['user_id'], 'idx_user'); diff --git a/src/Wallabag/CoreBundle/Controller/SiteCredentialController.php b/src/Wallabag/CoreBundle/Controller/SiteCredentialController.php index 0bacafb79..98781dab0 100644 --- a/src/Wallabag/CoreBundle/Controller/SiteCredentialController.php +++ b/src/Wallabag/CoreBundle/Controller/SiteCredentialController.php @@ -26,9 +26,9 @@ class SiteCredentialController extends Controller { $credentials = $this->get('wallabag_core.site_credential_repository')->findByUser($this->getUser()); - return $this->render('WallabagCoreBundle:SiteCredential:index.html.twig', array( + return $this->render('WallabagCoreBundle:SiteCredential:index.html.twig', [ 'credentials' => $credentials, - )); + ]); } /** @@ -36,6 +36,10 @@ class SiteCredentialController extends Controller * * @Route("/new", name="site_credentials_new") * @Method({"GET", "POST"}) + * + * @param Request $request + * + * @return \Symfony\Component\HttpFoundation\Response */ public function newAction(Request $request) { @@ -45,24 +49,25 @@ class SiteCredentialController extends Controller $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { + $credential->setUsername($this->get('wallabag_core.helper.crypto_proxy')->crypt($credential->getUsername())); $credential->setPassword($this->get('wallabag_core.helper.crypto_proxy')->crypt($credential->getPassword())); $em = $this->getDoctrine()->getManager(); $em->persist($credential); - $em->flush($credential); + $em->flush(); $this->get('session')->getFlashBag()->add( 'notice', $this->get('translator')->trans('flashes.site_credential.notice.added', ['%host%' => $credential->getHost()]) ); - return $this->redirectToRoute('site_credentials_edit', array('id' => $credential->getId())); + return $this->redirectToRoute('site_credentials_index'); } - return $this->render('WallabagCoreBundle:SiteCredential:new.html.twig', array( + return $this->render('WallabagCoreBundle:SiteCredential:new.html.twig', [ 'credential' => $credential, 'form' => $form->createView(), - )); + ]); } /** @@ -70,6 +75,11 @@ class SiteCredentialController extends Controller * * @Route("/{id}/edit", name="site_credentials_edit") * @Method({"GET", "POST"}) + * + * @param Request $request + * @param SiteCredential $siteCredential + * + * @return \Symfony\Component\HttpFoundation\Response */ public function editAction(Request $request, SiteCredential $siteCredential) { @@ -80,6 +90,9 @@ class SiteCredentialController extends Controller $editForm->handleRequest($request); if ($editForm->isSubmitted() && $editForm->isValid()) { + $siteCredential->setUsername($this->get('wallabag_core.helper.crypto_proxy')->crypt($siteCredential->getUsername())); + $siteCredential->setPassword($this->get('wallabag_core.helper.crypto_proxy')->crypt($siteCredential->getPassword())); + $em = $this->getDoctrine()->getManager(); $em->persist($siteCredential); $em->flush(); @@ -89,14 +102,14 @@ class SiteCredentialController extends Controller $this->get('translator')->trans('flashes.site_credential.notice.updated', ['%host%' => $siteCredential->getHost()]) ); - return $this->redirectToRoute('site_credentials_edit', array('id' => $siteCredential->getId())); + return $this->redirectToRoute('site_credentials_index'); } - return $this->render('WallabagCoreBundle:SiteCredential:edit.html.twig', array( + return $this->render('WallabagCoreBundle:SiteCredential:edit.html.twig', [ 'credential' => $siteCredential, 'edit_form' => $editForm->createView(), 'delete_form' => $deleteForm->createView(), - )); + ]); } /** @@ -104,6 +117,11 @@ class SiteCredentialController extends Controller * * @Route("/{id}", name="site_credentials_delete") * @Method("DELETE") + * + * @param Request $request + * @param SiteCredential $siteCredential + * + * @return \Symfony\Component\HttpFoundation\RedirectResponse */ public function deleteAction(Request $request, SiteCredential $siteCredential) { @@ -136,7 +154,7 @@ class SiteCredentialController extends Controller private function createDeleteForm(SiteCredential $siteCredential) { return $this->createFormBuilder() - ->setAction($this->generateUrl('site_credentials_delete', array('id' => $siteCredential->getId()))) + ->setAction($this->generateUrl('site_credentials_delete', ['id' => $siteCredential->getId()])) ->setMethod('DELETE') ->getForm() ; diff --git a/src/Wallabag/CoreBundle/Entity/SiteCredential.php b/src/Wallabag/CoreBundle/Entity/SiteCredential.php index 732d95066..58075e928 100644 --- a/src/Wallabag/CoreBundle/Entity/SiteCredential.php +++ b/src/Wallabag/CoreBundle/Entity/SiteCredential.php @@ -37,8 +37,7 @@ class SiteCredential * @var string * * @Assert\NotBlank() - * @Assert\Length(max=255) - * @ORM\Column(name="username", type="string", length=255) + * @ORM\Column(name="username", type="text") */ private $username; diff --git a/src/Wallabag/CoreBundle/Form/Type/SiteCredentialType.php b/src/Wallabag/CoreBundle/Form/Type/SiteCredentialType.php index 9db7c155d..fd409ad2c 100644 --- a/src/Wallabag/CoreBundle/Form/Type/SiteCredentialType.php +++ b/src/Wallabag/CoreBundle/Form/Type/SiteCredentialType.php @@ -19,6 +19,7 @@ class SiteCredentialType extends AbstractType ]) ->add('username', TextType::class, [ 'label' => 'site_credential.form.username_label', + 'data' => '', ]) ->add('password', PasswordType::class, [ 'label' => 'site_credential.form.password_label', diff --git a/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php b/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php index 62a3bc131..a79e6ebed 100644 --- a/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php +++ b/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php @@ -87,7 +87,8 @@ class GrabySiteConfigBuilder implements SiteConfigBuilder $config = new SiteConfig($parameters); - // do not leak password in log + // do not leak usernames and passwords in log + $parameters['username'] = '**masked**'; $parameters['password'] = '**masked**'; $this->logger->debug('Auth: add parameters.', ['host' => $host, 'parameters' => $parameters]); diff --git a/src/Wallabag/CoreBundle/Helper/CryptoProxy.php b/src/Wallabag/CoreBundle/Helper/CryptoProxy.php index d0a9b85c8..e8b19cb9e 100644 --- a/src/Wallabag/CoreBundle/Helper/CryptoProxy.php +++ b/src/Wallabag/CoreBundle/Helper/CryptoProxy.php @@ -65,7 +65,7 @@ class CryptoProxy /** * Load the private key. * - * @return string + * @return Key */ private function loadKey() { @@ -81,6 +81,6 @@ class CryptoProxy */ private function mask($value) { - return $value[0].'*****'.$value[strlen($value) - 1]; + return strlen($value) > 0 ? $value[0].'*****'.$value[strlen($value) - 1] : 'Empty value'; } } diff --git a/src/Wallabag/CoreBundle/Repository/SiteCredentialRepository.php b/src/Wallabag/CoreBundle/Repository/SiteCredentialRepository.php index 6f904f0ae..369067612 100644 --- a/src/Wallabag/CoreBundle/Repository/SiteCredentialRepository.php +++ b/src/Wallabag/CoreBundle/Repository/SiteCredentialRepository.php @@ -38,7 +38,8 @@ class SiteCredentialRepository extends \Doctrine\ORM\EntityRepository return; } - // decrypt password before returning it + // decrypt user & password before returning them + $res['username'] = $this->cryptoProxy->decrypt($res['username']); $res['password'] = $this->cryptoProxy->decrypt($res['password']); return $res; diff --git a/src/Wallabag/CoreBundle/Resources/translations/messages.fr.yml b/src/Wallabag/CoreBundle/Resources/translations/messages.fr.yml index 542ddf486..cd239b5cb 100644 --- a/src/Wallabag/CoreBundle/Resources/translations/messages.fr.yml +++ b/src/Wallabag/CoreBundle/Resources/translations/messages.fr.yml @@ -515,7 +515,7 @@ user: twofactor_label: "Double authentification" save: "Sauvegarder" delete: "Supprimer" - delete_confirm: "Êtes-vous sur ?" + delete_confirm: "Êtes-vous sûr ?" back_to_list: "Revenir à la liste" search: placeholder: "Filtrer par nom d’utilisateur ou email" @@ -537,7 +537,7 @@ site_credential: password_label: 'Mot de passe' save: "Sauvegarder" delete: "Supprimer" - delete_confirm: "Êtes-vous sur ?" + delete_confirm: "Êtes-vous sûr ?" back_to_list: "Revenir à la liste" error: diff --git a/src/Wallabag/CoreBundle/Resources/views/themes/material/SiteCredential/index.html.twig b/src/Wallabag/CoreBundle/Resources/views/themes/material/SiteCredential/index.html.twig index c128bcebd..4d30a6925 100644 --- a/src/Wallabag/CoreBundle/Resources/views/themes/material/SiteCredential/index.html.twig +++ b/src/Wallabag/CoreBundle/Resources/views/themes/material/SiteCredential/index.html.twig @@ -16,6 +16,7 @@ {{ 'site_credential.form.host_label'|trans }} {{ 'site_credential.form.username_label'|trans }} + {{ 'site_credential.form.password_label'|trans }} {{ 'site_credential.list.actions'|trans }} @@ -23,7 +24,8 @@ {% for credential in credentials %} {{ credential.host }} - {{ credential.username }} + ***** + ***** {{ 'site_credential.list.edit_action'|trans }} diff --git a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php index 803806853..f17dc97b3 100644 --- a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php @@ -1340,7 +1340,7 @@ class EntryControllerTest extends WallabagCoreTestCase $user = $client->getContainer()->get('security.token_storage')->getToken()->getUser(); $credential = new SiteCredential($user); $credential->setHost('monde-diplomatique.fr'); - $credential->setUsername('foo'); + $credential->setUsername($client->getContainer()->get('wallabag_core.helper.crypto_proxy')->crypt('foo')); $credential->setPassword($client->getContainer()->get('wallabag_core.helper.crypto_proxy')->crypt('bar')); $em->persist($credential); diff --git a/tests/Wallabag/CoreBundle/Controller/SiteCredentialControllerTest.php b/tests/Wallabag/CoreBundle/Controller/SiteCredentialControllerTest.php index 7e6dafee6..e73a9743c 100644 --- a/tests/Wallabag/CoreBundle/Controller/SiteCredentialControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/SiteCredentialControllerTest.php @@ -85,7 +85,6 @@ class SiteCredentialControllerTest extends WallabagCoreTestCase $crawler = $client->followRedirect(); $this->assertContains('flashes.site_credential.notice.updated', $crawler->filter('body')->extract(['_text'])[0]); - $this->assertContains('larry', $crawler->filter('input[id=site_credential_username]')->attr('value')); } public function testEditFromADifferentUserSiteCredential()