Fix reviews

Encrypt username too
Redirect to list after saving credentials
Fix typos

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

diff --git a/app/DoctrineMigrations/Version20170501115751.php b/app/DoctrineMigrations/Version20170501115751.php
index 2597f1ffd2dcbf48cd2e8bd77681a84a85ed302a..7f068eb84e7cf93fdb1a3a69d3b7663d3b147f7b 100644 (file)
--- 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 0bacafb79a24ec6c7d46a0d35d6484571f4e5b44..98781dab0f84a071e09b4621b0e960ffb6fdb218 100644 (file)
--- 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 732d95066a3a167ad9f99f1d3b75518f5750ad22..58075e928234f18383467318538b9718f5c5d77d 100644 (file)
--- 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 9db7c155dc0978b4b164cb4c6cac2a1d29eaa539..fd409ad2c3d1832431dc78b7e05788728471778e 100644 (file)
--- 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 62a3bc1319fc0bf89a7a69d6e2c984a605ec7ba4..a79e6ebed64712c117ca16d691f6c34fcd1c8363 100644 (file)
--- 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 d0a9b85c83882d15d4bd4cc51308e2d54e61357f..e8b19cb9ec650667cc351edb1bf11ea4642a388c 100644 (file)
--- 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 6f904f0ae86b2c8cc7e8236378e7e2f92578243a..36906761243616bd458c5f83682a0be92c165751 100644 (file)
--- 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 542ddf486274b04db1a43337808f11f24f384e66..cd239b5cbd989c0cda0e582469776aeb6cc59971 100644 (file)
--- 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 c128bcebd0e13b3f6cf176e245343421c62999f5..4d30a69254930577c659b590ab878d3635b5575e 100644 (file)
--- 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 @@
                                 <tr>
                                     <th>{{ 'site_credential.form.host_label'|trans }}</th>
                                     <th>{{ 'site_credential.form.username_label'|trans }}</th>
+                                    <th>{{ 'site_credential.form.password_label'|trans }}</th>
                                     <th>{{ 'site_credential.list.actions'|trans }}</th>
                                 </tr>
                             </thead>
@@ -23,7 +24,8 @@
                             {% for credential in credentials %}
                                 <tr>
                                     <td>{{ credential.host }}</td>
-                                    <td>{{ credential.username }}</td>
+                                    <td>*****</td>
+                                    <td>*****</td>
                                     <td>
                                         <a href="{{ path('site_credentials_edit', { 'id': credential.id }) }}">{{ 'site_credential.list.edit_action'|trans }}</a>
                                     </td>

diff --git a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php
index 803806853883ad6cc00eb979c97b4450cb5a9cf6..f17dc97b384755683a3277435c1affe2cf3d617d 100644 (file)
--- 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 7e6dafee654539d18a4d619a3fc710d245c85cb7..e73a9743c0e470a89a810b5ca5d98d76f4567b35 100644 (file)
--- 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()