Merge pull request #3101 from wallabag/credentials-new-extrafields

Skip auth when no credentials are found

4 files changed, 58 insertions, 22 deletions

diff --git a/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php b/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php
index 1c866f17..c712bb26 100644
--- a/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php
+++ b/src/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilder.php
@@ -6,28 +6,35 @@ use BD\GuzzleSiteAuthenticator\SiteConfig\SiteConfig;
 use BD\GuzzleSiteAuthenticator\SiteConfig\SiteConfigBuilder;
 use Graby\SiteConfig\ConfigBuilder;
 use OutOfRangeException;
+use Psr\Log\LoggerInterface;
 
 class GrabySiteConfigBuilder implements SiteConfigBuilder
 {
     /**
-     * @var \Graby\SiteConfig\ConfigBuilder
+     * @var ConfigBuilder
      */
     private $grabyConfigBuilder;
     /**
      * @var array
      */
     private $credentials;
+    /**
+     * @var LoggerInterface
+     */
+    private $logger;
 
     /**
      * GrabySiteConfigBuilder constructor.
      *
-     * @param \Graby\SiteConfig\ConfigBuilder $grabyConfigBuilder
-     * @param array                           $credentials
+     * @param ConfigBuilder   $grabyConfigBuilder
+     * @param array           $credentials
+     * @param LoggerInterface $logger
      */
-    public function __construct(ConfigBuilder $grabyConfigBuilder, array $credentials = [])
+    public function __construct(ConfigBuilder $grabyConfigBuilder, array $credentials, LoggerInterface $logger)
     {
         $this->grabyConfigBuilder = $grabyConfigBuilder;
         $this->credentials = $credentials;
+        $this->logger = $logger;
     }
 
     /**
@@ -47,6 +54,12 @@ class GrabySiteConfigBuilder implements SiteConfigBuilder
             $host = substr($host, 4);
         }
 
+        if (empty($this->credentials[$host])) {
+            $this->logger->debug('Auth: no credentials available for host.', ['host' => $host]);
+
+            return false;
+        }
+
         $config = $this->grabyConfigBuilder->buildForHost($host);
         $parameters = [
             'host' => $host,
@@ -56,14 +69,18 @@ class GrabySiteConfigBuilder implements SiteConfigBuilder
             'passwordField' => $config->login_password_field ?: null,
             'extraFields' => $this->processExtraFields($config->login_extra_fields),
             'notLoggedInXpath' => $config->not_logged_in_xpath ?: null,
+            'username' => $this->credentials[$host]['username'],
+            'password' => $this->credentials[$host]['password'],
         ];
 
-        if (isset($this->credentials[$host])) {
-            $parameters['username'] = $this->credentials[$host]['username'];
-            $parameters['password'] = $this->credentials[$host]['password'];
-        }
+        $config = new SiteConfig($parameters);
+
+        // do not leak password in log
+        $parameters['password'] = '**masked**';
 
-        return new SiteConfig($parameters);
+        $this->logger->debug('Auth: add parameters.', ['host' => $host, 'parameters' => $parameters]);
+
+        return $config;
     }
 
     /**
@@ -85,6 +102,7 @@ class GrabySiteConfigBuilder implements SiteConfigBuilder
             if (strpos($extraField, '=') === false) {
                 continue;
             }
+
             list($fieldName, $fieldValue) = explode('=', $extraField, 2);
             $extraFields[$fieldName] = $fieldValue;
         }
diff --git a/src/Wallabag/CoreBundle/Helper/HttpClientFactory.php b/src/Wallabag/CoreBundle/Helper/HttpClientFactory.php
index 11ef26d8..43f5b119 100644
--- a/src/Wallabag/CoreBundle/Helper/HttpClientFactory.php
+++ b/src/Wallabag/CoreBundle/Helper/HttpClientFactory.php
@@ -51,6 +51,7 @@ class HttpClientFactory
         $this->cookieJar->clear();
         // need to set the (shared) cookie jar
         $client = new Client(['handler' => new SafeCurlHandler(), 'defaults' => ['cookies' => $this->cookieJar]]);
+
         foreach ($this->subscribers as $subscriber) {
             $client->getEmitter()->attach($subscriber);
         }
diff --git a/src/Wallabag/CoreBundle/Resources/config/services.yml b/src/Wallabag/CoreBundle/Resources/config/services.yml
index 68f900a1..6c9195ce 100644
--- a/src/Wallabag/CoreBundle/Resources/config/services.yml
+++ b/src/Wallabag/CoreBundle/Resources/config/services.yml
@@ -63,6 +63,9 @@ services:
         arguments:
             - "@wallabag_core.graby.config_builder"
             - "%sites_credentials%"
+            - '@logger'
+        tags:
+            - { name: monolog.logger, channel: graby }
 
     # service alias override
     bd_guzzle_site_authenticator.site_config_builder:
diff --git a/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php b/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php
index 8341b11f..8b50bce9 100644
--- a/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php
+++ b/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php
@@ -2,6 +2,8 @@
 
 namespace Tests\Wallabag\CoreBundle\GuzzleSiteAuthenticator;
 
+use Monolog\Handler\TestHandler;
+use Monolog\Logger;
 use BD\GuzzleSiteAuthenticator\SiteConfig\SiteConfig;
 use Graby\SiteConfig\SiteConfig as GrabySiteConfig;
 use PHPUnit_Framework_TestCase;
@@ -32,14 +34,19 @@ class GrabySiteConfigBuilderTest extends PHPUnit_Framework_TestCase
             ->with('example.com')
             ->will($this->returnValue($grabySiteConfig));
 
+        $logger = new Logger('foo');
+        $handler = new TestHandler();
+        $logger->pushHandler($handler);
+
         $this->builder = new GrabySiteConfigBuilder(
             $grabyConfigBuilderMock,
-            ['example.com' => ['username' => 'foo', 'password' => 'bar']]
+            ['example.com' => ['username' => 'foo', 'password' => 'bar']],
+            $logger
         );
 
         $config = $this->builder->buildForHost('example.com');
 
-        self::assertEquals(
+        $this->assertEquals(
             new SiteConfig([
                 'host' => 'example.com',
                 'requiresLogin' => true,
@@ -53,6 +60,10 @@ class GrabySiteConfigBuilderTest extends PHPUnit_Framework_TestCase
             ]),
             $config
         );
+
+        $records = $handler->getRecords();
+
+        $this->assertCount(1, $records, 'One log was recorded');
     }
 
     public function testBuildConfigDoesntExist()
@@ -67,19 +78,22 @@ class GrabySiteConfigBuilderTest extends PHPUnit_Framework_TestCase
             ->with('unknown.com')
             ->will($this->returnValue(new GrabySiteConfig()));
 
-        $this->builder = new GrabySiteConfigBuilder($grabyConfigBuilderMock, []);
+        $logger = new Logger('foo');
+        $handler = new TestHandler();
+        $logger->pushHandler($handler);
+
+        $this->builder = new GrabySiteConfigBuilder(
+            $grabyConfigBuilderMock,
+            [],
+            $logger
+        );
 
         $config = $this->builder->buildForHost('unknown.com');
 
-        self::assertEquals(
-            new SiteConfig([
-                'host' => 'unknown.com',
-                'requiresLogin' => false,
-                'username' => null,
-                'password' => null,
-                'extraFields' => [],
-            ]),
-            $config
-        );
+        $this->assertFalse($config);
+
+        $records = $handler->getRecords();
+
+        $this->assertCount(1, $records, 'One log was recorded');
     }
 }